From 641132574693441b83e3d07861864dad8e7df0c4 Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Tue, 5 May 2026 10:28:12 +0100 Subject: [PATCH 1/6] Add discussion comment write operation tools Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 23 + .../__toolsnaps__/add_discussion_comment.snap | 38 + .../delete_discussion_comment.snap | 19 + .../get_discussion_comments.snap | 4 + .../set_discussion_comment_answer.snap | 24 + .../update_discussion_comment.snap | 24 + pkg/github/discussions.go | 506 +++++++++- pkg/github/discussions_test.go | 943 +++++++++++++++++- pkg/github/minimal_types.go | 9 + pkg/github/tools.go | 4 + 10 files changed, 1556 insertions(+), 38 deletions(-) create mode 100644 pkg/github/__toolsnaps__/add_discussion_comment.snap create mode 100644 pkg/github/__toolsnaps__/delete_discussion_comment.snap create mode 100644 pkg/github/__toolsnaps__/set_discussion_comment_answer.snap create mode 100644 pkg/github/__toolsnaps__/update_discussion_comment.snap diff --git a/README.md b/README.md index 5f9baa780e..8850a2ac4c 100644 --- a/README.md +++ b/README.md @@ -730,6 +730,18 @@ The following sets of tools are available: comment-discussion Discussions +- **add_discussion_comment** - Add discussion comment + - **Required OAuth Scopes**: `repo` + - `body`: Comment content (string, required) + - `discussionNumber`: Discussion Number (number, required) + - `owner`: Repository owner (string, required) + - `replyToCommentNodeID`: The Node ID of the comment to reply to. If provided, the comment will be posted as a reply. (string, optional) + - `repo`: Repository name (string, required) + +- **delete_discussion_comment** - Delete discussion comment + - **Required OAuth Scopes**: `repo` + - `commentNodeID`: The Node ID of the discussion comment to delete (string, required) + - **get_discussion** - Get discussion - **Required OAuth Scopes**: `repo` - `discussionNumber`: Discussion Number (number, required) @@ -740,6 +752,7 @@ The following sets of tools are available: - **Required OAuth Scopes**: `repo` - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `discussionNumber`: Discussion Number (number, required) + - `includeReplies`: When true, each top-level comment will include its replies nested within it. Defaults to false. (boolean, optional) - `owner`: Repository owner (string, required) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) @@ -759,6 +772,16 @@ The following sets of tools are available: - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name. If not provided, discussions will be queried at the organisation level. (string, optional) +- **set_discussion_comment_answer** - Set discussion comment as answer + - **Required OAuth Scopes**: `repo` + - `commentNodeID`: The Node ID of the discussion comment to mark or unmark as the answer (string, required) + - `isAnswer`: Whether the comment is the answer to the discussion (true to mark, false to unmark) (boolean, required) + +- **update_discussion_comment** - Update discussion comment + - **Required OAuth Scopes**: `repo` + - `body`: The new contents of the comment (string, required) + - `commentNodeID`: The Node ID of the discussion comment to update (string, required) +
diff --git a/pkg/github/__toolsnaps__/add_discussion_comment.snap b/pkg/github/__toolsnaps__/add_discussion_comment.snap new file mode 100644 index 0000000000..e2044edbe4 --- /dev/null +++ b/pkg/github/__toolsnaps__/add_discussion_comment.snap @@ -0,0 +1,38 @@ +{ + "annotations": { + "title": "Add discussion comment" + }, + "description": "Add a comment to a discussion", + "inputSchema": { + "properties": { + "body": { + "description": "Comment content", + "type": "string" + }, + "discussionNumber": { + "description": "Discussion Number", + "type": "number" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "replyToCommentNodeID": { + "description": "The Node ID of the comment to reply to. If provided, the comment will be posted as a reply.", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "discussionNumber", + "body" + ], + "type": "object" + }, + "name": "add_discussion_comment" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/delete_discussion_comment.snap b/pkg/github/__toolsnaps__/delete_discussion_comment.snap new file mode 100644 index 0000000000..43cee5d429 --- /dev/null +++ b/pkg/github/__toolsnaps__/delete_discussion_comment.snap @@ -0,0 +1,19 @@ +{ + "annotations": { + "title": "Delete discussion comment" + }, + "description": "Delete a comment on a discussion", + "inputSchema": { + "properties": { + "commentNodeID": { + "description": "The Node ID of the discussion comment to delete", + "type": "string" + } + }, + "required": [ + "commentNodeID" + ], + "type": "object" + }, + "name": "delete_discussion_comment" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/get_discussion_comments.snap b/pkg/github/__toolsnaps__/get_discussion_comments.snap index f9e6095650..12f15f2097 100644 --- a/pkg/github/__toolsnaps__/get_discussion_comments.snap +++ b/pkg/github/__toolsnaps__/get_discussion_comments.snap @@ -14,6 +14,10 @@ "description": "Discussion Number", "type": "number" }, + "includeReplies": { + "description": "When true, each top-level comment will include its replies nested within it. Defaults to false.", + "type": "boolean" + }, "owner": { "description": "Repository owner", "type": "string" diff --git a/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap b/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap new file mode 100644 index 0000000000..a631ad305c --- /dev/null +++ b/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap @@ -0,0 +1,24 @@ +{ + "annotations": { + "title": "Set discussion comment as answer" + }, + "description": "Marks or unmarks a given discussion comment as the answer to the discussion.", + "inputSchema": { + "properties": { + "commentNodeID": { + "description": "The Node ID of the discussion comment to mark or unmark as the answer", + "type": "string" + }, + "isAnswer": { + "description": "Whether the comment is the answer to the discussion (true to mark, false to unmark)", + "type": "boolean" + } + }, + "required": [ + "commentNodeID", + "isAnswer" + ], + "type": "object" + }, + "name": "set_discussion_comment_answer" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_discussion_comment.snap b/pkg/github/__toolsnaps__/update_discussion_comment.snap new file mode 100644 index 0000000000..262f1b5710 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_discussion_comment.snap @@ -0,0 +1,24 @@ +{ + "annotations": { + "title": "Update discussion comment" + }, + "description": "Update a comment on a discussion", + "inputSchema": { + "properties": { + "body": { + "description": "The new contents of the comment", + "type": "string" + }, + "commentNodeID": { + "description": "The Node ID of the discussion comment to update", + "type": "string" + } + }, + "required": [ + "commentNodeID", + "body" + ], + "type": "object" + }, + "name": "update_discussion_comment" +} \ No newline at end of file diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 700560b475..7f79354a4a 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "strings" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/scopes" @@ -405,6 +406,10 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve Type: "number", Description: "Discussion Number", }, + "includeReplies": { + Type: "boolean", + Description: "When true, each top-level comment will include its replies nested within it. Defaults to false.", + }, }, Required: []string{"owner", "repo", "discussionNumber"}, }), @@ -421,6 +426,11 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve return utils.NewToolResultError(err.Error()), nil, nil } + includeReplies, err := OptionalParam[bool](args, "includeReplies") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + // Get pagination parameters and convert to GraphQL format pagination, err := OptionalCursorPaginationParams(args) if err != nil { @@ -447,24 +457,6 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil } - var q struct { - Repository struct { - Discussion struct { - Comments struct { - Nodes []struct { - Body githubv4.String - } - PageInfo struct { - HasNextPage githubv4.Boolean - HasPreviousPage githubv4.Boolean - StartCursor githubv4.String - EndCursor githubv4.String - } - TotalCount int - } `graphql:"comments(first: $first, after: $after)"` - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } vars := map[string]any{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), @@ -476,25 +468,109 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve } else { vars["after"] = (*githubv4.String)(nil) } - if err := client.Query(ctx, &q, vars); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + + var comments []MinimalDiscussionComment + var pageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String } + var totalCount int - var comments []*github.IssueComment - for _, c := range q.Repository.Discussion.Comments.Nodes { - comments = append(comments, &github.IssueComment{Body: github.Ptr(string(c.Body))}) + if includeReplies { + var q struct { + Repository struct { + Discussion struct { + Comments struct { + Nodes []struct { + ID githubv4.ID + Body githubv4.String + IsAnswer githubv4.Boolean + Replies struct { + Nodes []struct { + ID githubv4.ID + Body githubv4.String + } + TotalCount int + } `graphql:"replies(first: 100)"` + } + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + TotalCount int + } `graphql:"comments(first: $first, after: $after)"` + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + if err := client.Query(ctx, &q, vars); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + for _, c := range q.Repository.Discussion.Comments.Nodes { + comment := MinimalDiscussionComment{ + ID: fmt.Sprintf("%v", c.ID), + Body: string(c.Body), + IsAnswer: bool(c.IsAnswer), + ReplyTotalCount: c.Replies.TotalCount, + } + for _, r := range c.Replies.Nodes { + comment.Replies = append(comment.Replies, MinimalDiscussionComment{ + ID: fmt.Sprintf("%v", r.ID), + Body: string(r.Body), + }) + } + comments = append(comments, comment) + } + pageInfo = q.Repository.Discussion.Comments.PageInfo + totalCount = q.Repository.Discussion.Comments.TotalCount + } else { + var q struct { + Repository struct { + Discussion struct { + Comments struct { + Nodes []struct { + ID githubv4.ID + Body githubv4.String + IsAnswer githubv4.Boolean + } + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + TotalCount int + } `graphql:"comments(first: $first, after: $after)"` + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + if err := client.Query(ctx, &q, vars); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + for _, c := range q.Repository.Discussion.Comments.Nodes { + comments = append(comments, MinimalDiscussionComment{ + ID: fmt.Sprintf("%v", c.ID), + Body: string(c.Body), + IsAnswer: bool(c.IsAnswer), + }) + } + pageInfo = q.Repository.Discussion.Comments.PageInfo + totalCount = q.Repository.Discussion.Comments.TotalCount } // Create response with pagination info response := map[string]any{ "comments": comments, "pageInfo": map[string]any{ - "hasNextPage": q.Repository.Discussion.Comments.PageInfo.HasNextPage, - "hasPreviousPage": q.Repository.Discussion.Comments.PageInfo.HasPreviousPage, - "startCursor": string(q.Repository.Discussion.Comments.PageInfo.StartCursor), - "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), + "hasNextPage": pageInfo.HasNextPage, + "hasPreviousPage": pageInfo.HasPreviousPage, + "startCursor": string(pageInfo.StartCursor), + "endCursor": string(pageInfo.EndCursor), }, - "totalCount": q.Repository.Discussion.Comments.TotalCount, + "totalCount": totalCount, } out, err := json.Marshal(response) @@ -507,6 +583,380 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve ) } +func AddDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataDiscussions, + mcp.Tool{ + Name: "add_discussion_comment", + Description: t("TOOL_ADD_DISCUSSION_COMMENT_DESCRIPTION", "Add a comment to a discussion"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ADD_DISCUSSION_COMMENT_USER_TITLE", "Add discussion comment"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "discussionNumber": { + Type: "number", + Description: "Discussion Number", + }, + "body": { + Type: "string", + Description: "Comment content", + }, + "replyToCommentNodeID": { + Type: "string", + Description: "The Node ID of the comment to reply to. If provided, the comment will be posted as a reply.", + }, + }, + Required: []string{"owner", "repo", "discussionNumber", "body"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // Decode params + var params struct { + Owner string + Repo string + DiscussionNumber int32 + Body string + } + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + } + + // Get the discussion's node ID using its number + var q struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + vars := map[string]any{ + "owner": githubv4.String(params.Owner), + "repo": githubv4.String(params.Repo), + "discussionNumber": githubv4.Int(params.DiscussionNumber), + } + if err := client.Query(ctx, &q, vars); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + // Add the comment using the discussion's node ID + input := githubv4.AddDiscussionCommentInput{ + DiscussionID: q.Repository.Discussion.ID, + Body: githubv4.String(params.Body), + } + + replyToCommentNodeID, err := OptionalParam[string](args, "replyToCommentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if replyToCommentNodeID != "" { + if strings.TrimSpace(replyToCommentNodeID) == "" { + return utils.NewToolResultError("replyToCommentNodeID cannot be blank"), nil, nil + } + // The GitHub API silently ignores an invalid ReplyToID and creates a top-level + // comment instead of returning an error, so we validate upfront that the node + // exists and is a DiscussionComment to give callers a clear failure. + var nodeQuery struct { + Node struct { + DiscussionComment struct { + ID githubv4.ID + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + } + if err := client.Query(ctx, &nodeQuery, map[string]any{ + "replyToID": githubv4.ID(replyToCommentNodeID), + }); err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to validate replyToCommentNodeID: %v", err)), nil, nil + } + if nodeQuery.Node.DiscussionComment.ID == nil || nodeQuery.Node.DiscussionComment.ID == "" { + return utils.NewToolResultError(fmt.Sprintf("replyToCommentNodeID %q does not resolve to a valid discussion comment", replyToCommentNodeID)), nil, nil + } + id := githubv4.ID(replyToCommentNodeID) + input.ReplyToID = &id + } + + var mutation struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"addDiscussionComment(input: $input)"` + } + + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + comment := mutation.AddDiscussionComment.Comment + minimalResponse := MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + } + + out, err := json.Marshal(minimalResponse) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil + }) +} + +func UpdateDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataDiscussions, + mcp.Tool{ + Name: "update_discussion_comment", + Description: t("TOOL_UPDATE_DISCUSSION_COMMENT_DESCRIPTION", "Update a comment on a discussion"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UPDATE_DISCUSSION_COMMENT_USER_TITLE", "Update discussion comment"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "commentNodeID": { + Type: "string", + Description: "The Node ID of the discussion comment to update", + }, + "body": { + Type: "string", + Description: "The new contents of the comment", + }, + }, + Required: []string{"commentNodeID", "body"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + var params struct { + CommentNodeID string + Body string + } + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + } + + input := githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID(params.CommentNodeID), + Body: githubv4.String(params.Body), + } + + var mutation struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + } + + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + comment := mutation.UpdateDiscussionComment.Comment + minimalResponse := MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + } + + out, err := json.Marshal(minimalResponse) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil + }) +} + +func DeleteDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataDiscussions, + mcp.Tool{ + Name: "delete_discussion_comment", + Description: t("TOOL_DELETE_DISCUSSION_COMMENT_DESCRIPTION", "Delete a comment on a discussion"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_DELETE_DISCUSSION_COMMENT_USER_TITLE", "Delete discussion comment"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "commentNodeID": { + Type: "string", + Description: "The Node ID of the discussion comment to delete", + }, + }, + Required: []string{"commentNodeID"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + var params struct { + CommentNodeID string + } + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + } + + input := githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID(params.CommentNodeID), + } + + var mutation struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + } + + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + comment := mutation.DeleteDiscussionComment.Comment + minimalResponse := MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + } + + out, err := json.Marshal(minimalResponse) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil + }) +} + +func SetDiscussionCommentAnswer(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataDiscussions, + mcp.Tool{ + Name: "set_discussion_comment_answer", + Description: t("TOOL_SET_DISCUSSION_COMMENT_ANSWER_DESCRIPTION", "Marks or unmarks a given discussion comment as the answer to the discussion."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_SET_DISCUSSION_COMMENT_ANSWER_USER_TITLE", "Set discussion comment as answer"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "commentNodeID": { + Type: "string", + Description: "The Node ID of the discussion comment to mark or unmark as the answer", + }, + "isAnswer": { + Type: "boolean", + Description: "Whether the comment is the answer to the discussion (true to mark, false to unmark)", + }, + }, + Required: []string{"commentNodeID", "isAnswer"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if _, ok := args["isAnswer"]; !ok { + return utils.NewToolResultError("isAnswer is required"), nil, nil + } + isAnswer, err := OptionalParam[bool](args, "isAnswer") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + } + + var discussionID githubv4.ID + var discussionURL githubv4.String + + if isAnswer { + input := githubv4.MarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID(commentNodeID), + } + var mutation struct { + MarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + discussionID = mutation.MarkDiscussionCommentAsAnswer.Discussion.ID + discussionURL = mutation.MarkDiscussionCommentAsAnswer.Discussion.URL + } else { + input := githubv4.UnmarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID(commentNodeID), + } + var mutation struct { + UnmarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + discussionID = mutation.UnmarkDiscussionCommentAsAnswer.Discussion.ID + discussionURL = mutation.UnmarkDiscussionCommentAsAnswer.Discussion.URL + } + + response := struct { + DiscussionID string `json:"discussionID"` + DiscussionURL string `json:"discussionURL"` + }{ + DiscussionID: fmt.Sprintf("%v", discussionID), + DiscussionURL: string(discussionURL), + } + + out, err := json.Marshal(response) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal discussion: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil + }, + ) +} + func ListDiscussionCategories(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( ToolsetMetadataDiscussions, diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 692ef2ec83..bb971b4da0 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -647,10 +647,11 @@ func Test_GetDiscussionComments(t *testing.T) { assert.Contains(t, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "discussionNumber") + assert.Contains(t, schema.Properties, "includeReplies") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "discussionNumber"}) // Use exact string query that matches implementation output - qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{body},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" + qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]any{ @@ -666,8 +667,8 @@ func Test_GetDiscussionComments(t *testing.T) { "discussion": map[string]any{ "comments": map[string]any{ "nodes": []map[string]any{ - {"body": "This is the first comment"}, - {"body": "This is the second comment"}, + {"id": "DC_id1", "body": "This is the first comment"}, + {"id": "DC_id2", "body": "This is the second comment"}, }, "pageInfo": map[string]any{ "hasNextPage": false, @@ -701,7 +702,10 @@ func Test_GetDiscussionComments(t *testing.T) { // (Lines removed) var response struct { - Comments []*github.IssueComment `json:"comments"` + Comments []struct { + ID string `json:"id"` + Body string `json:"body"` + } `json:"comments"` PageInfo struct { HasNextPage bool `json:"hasNextPage"` HasPreviousPage bool `json:"hasPreviousPage"` @@ -713,17 +717,17 @@ func Test_GetDiscussionComments(t *testing.T) { err = json.Unmarshal([]byte(textContent.Text), &response) require.NoError(t, err) assert.Len(t, response.Comments, 2) - expectedBodies := []string{"This is the first comment", "This is the second comment"} - for i, comment := range response.Comments { - assert.Equal(t, expectedBodies[i], *comment.Body) - } + assert.Equal(t, "DC_id1", response.Comments[0].ID) + assert.Equal(t, "This is the first comment", response.Comments[0].Body) + assert.Equal(t, "DC_id2", response.Comments[1].ID) + assert.Equal(t, "This is the second comment", response.Comments[1].Body) } func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) { // Test that WeakDecode handles string discussionNumber from MCP clients toolDef := GetDiscussionComments(translations.NullTranslationHelper) - qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{body},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" + qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" vars := map[string]any{ "owner": "owner", @@ -738,7 +742,7 @@ func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) { "discussion": map[string]any{ "comments": map[string]any{ "nodes": []map[string]any{ - {"body": "First comment"}, + {"id": "DC_id3", "body": "First comment"}, }, "pageInfo": map[string]any{ "hasNextPage": false, @@ -777,6 +781,7 @@ func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) { } require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out)) assert.Len(t, out.Comments, 1) + assert.Equal(t, "DC_id3", out.Comments[0]["id"]) assert.Equal(t, "First comment", out.Comments[0]["body"]) } @@ -924,3 +929,921 @@ func Test_ListDiscussionCategories(t *testing.T) { }) } } + +func Test_AddDiscussionComment(t *testing.T) { + t.Parallel() + + // Verify tool definition and schema + toolDef := AddDiscussionComment(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "add_discussion_comment", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint, "add_discussion_comment should not be read-only") + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Contains(t, schema.Properties, "owner") + assert.Contains(t, schema.Properties, "repo") + assert.Contains(t, schema.Properties, "discussionNumber") + assert.Contains(t, schema.Properties, "body") + assert.Contains(t, schema.Properties, "replyToCommentNodeID") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "discussionNumber", "body"}) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedID string + expectedURL string + }{ + { + name: "successful comment creation", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a test comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "discussionNumber": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"addDiscussionComment(input: $input)"` + }{}, + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a test comment"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + expectToolError: false, + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + { + name: "discussion not found", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(999), + "body": "This is a comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "discussionNumber": githubv4.Int(999), + }, + githubv4mock.ErrorResponse("Could not resolve to a Discussion with the number of 999."), + ), + ), + expectToolError: true, + expectedErrMsg: "Could not resolve to a Discussion with the number of 999.", + }, + { + name: "mutation failure", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "discussionNumber": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"addDiscussionComment(input: $input)"` + }{}, + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a comment"), + }, + nil, + githubv4mock.ErrorResponse("insufficient permissions to comment on this discussion"), + ), + ), + expectToolError: true, + expectedErrMsg: "insufficient permissions to comment on this discussion", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + req := createMCPRequest(tc.requestArgs) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + + if tc.expectToolError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.expectedErrMsg) + return + } + + require.False(t, res.IsError) + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedID, response.ID) + assert.Equal(t, tc.expectedURL, response.URL) + }) + } +} + +func Test_AddDiscussionCommentReply(t *testing.T) { + t.Parallel() + + toolDef := AddDiscussionComment(translations.NullTranslationHelper) + + queryMatcher := githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "discussionNumber": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + }, + }, + }), + ) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedID string + expectedURL string + }{ + { + name: "successful reply to comment", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "replyToCommentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + queryMatcher, + githubv4mock.NewQueryMatcher( + struct { + Node struct { + DiscussionComment struct { + ID githubv4.ID + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + }{}, + map[string]any{ + "replyToID": githubv4.ID("DC_kwDOComment456"), + }, + githubv4mock.DataResponse(map[string]any{ + "node": map[string]any{ + "id": "DC_kwDOComment456", + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"addDiscussionComment(input: $input)"` + }{}, + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a reply"), + ReplyToID: githubv4ptr("DC_kwDOComment456"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOReply789", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-789", + }, + }, + }), + ), + ), + expectToolError: false, + expectedID: "DC_kwDOReply789", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-789", + }, + { + name: "whitespace-only replyToCommentNodeID is rejected", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "replyToCommentNodeID": " ", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(queryMatcher), + expectToolError: true, + expectedErrMsg: "replyToCommentNodeID cannot be blank", + }, + { + name: "invalid replyToCommentNodeID returns error", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "replyToCommentNodeID": "DC_kwDOInvalid", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + queryMatcher, + githubv4mock.NewQueryMatcher( + struct { + Node struct { + DiscussionComment struct { + ID githubv4.ID + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + }{}, + map[string]any{ + "replyToID": githubv4.ID("DC_kwDOInvalid"), + }, + githubv4mock.DataResponse(map[string]any{ + "node": nil, + }), + ), + ), + expectToolError: true, + expectedErrMsg: `replyToCommentNodeID "DC_kwDOInvalid" does not resolve to a valid discussion comment`, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + req := createMCPRequest(tc.requestArgs) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + + if tc.expectToolError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.expectedErrMsg) + return + } + + require.False(t, res.IsError) + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedID, response.ID) + assert.Equal(t, tc.expectedURL, response.URL) + }) + } +} + +func githubv4ptr(id githubv4.ID) *githubv4.ID { + return &id +} + +func Test_GetDiscussionCommentsWithReplies(t *testing.T) { + t.Parallel() + + toolDef := GetDiscussionComments(translations.NullTranslationHelper) + + qWithReplies := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer,replies(first: 100){nodes{id,body},totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" + + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + "first": float64(30), + "after": (*string)(nil), + } + + mockResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "comments": map[string]any{ + "nodes": []map[string]any{ + { + "id": "DC_id1", + "body": "Top-level comment", + "replies": map[string]any{ + "nodes": []map[string]any{ + {"id": "DC_reply1", "body": "Reply to first comment"}, + }, + "totalCount": 1, + }, + }, + { + "id": "DC_id2", + "body": "Another top-level comment", + "replies": map[string]any{ + "nodes": []map[string]any{}, + "totalCount": 0, + }, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, + }, + }, + }, + }) + + matcher := githubv4mock.NewQueryMatcher(qWithReplies, vars, mockResponse) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + reqParams := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "includeReplies": true, + } + req := createMCPRequest(reqParams) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + require.False(t, res.IsError, "expected no error, got: %s", text) + + var response struct { + Comments []MinimalDiscussionComment `json:"comments"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + } `json:"pageInfo"` + TotalCount int `json:"totalCount"` + } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Len(t, response.Comments, 2) + + assert.Equal(t, "DC_id1", response.Comments[0].ID) + assert.Equal(t, "Top-level comment", response.Comments[0].Body) + require.Len(t, response.Comments[0].Replies, 1) + assert.Equal(t, "DC_reply1", response.Comments[0].Replies[0].ID) + assert.Equal(t, "Reply to first comment", response.Comments[0].Replies[0].Body) + assert.Equal(t, 1, response.Comments[0].ReplyTotalCount) + + assert.Equal(t, "DC_id2", response.Comments[1].ID) + assert.Empty(t, response.Comments[1].Replies) + assert.Equal(t, 0, response.Comments[1].ReplyTotalCount) +} + +func Test_UpdateDiscussionComment(t *testing.T) { + t.Parallel() + + // Verify tool definition and schema + toolDef := UpdateDiscussionComment(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "update_discussion_comment", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint, "update_discussion_comment should not be read-only") + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Contains(t, schema.Properties, "commentNodeID") + assert.Contains(t, schema.Properties, "body") + assert.ElementsMatch(t, schema.Required, []string{"commentNodeID", "body"}) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedID string + expectedURL string + }{ + { + name: "successful comment update", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOComment456"), + Body: githubv4.String("Updated comment text"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + expectToolError: false, + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + { + name: "comment not found", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOInvalid", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOInvalid"), + Body: githubv4.String("Updated comment text"), + }, + nil, + githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOInvalid'."), + ), + ), + expectToolError: true, + expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOInvalid'.", + }, + { + name: "insufficient permissions", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOComment456"), + Body: githubv4.String("Updated comment text"), + }, + nil, + githubv4mock.ErrorResponse("insufficient permissions to update this discussion comment"), + ), + ), + expectToolError: true, + expectedErrMsg: "insufficient permissions to update this discussion comment", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + req := createMCPRequest(tc.requestArgs) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + + if tc.expectToolError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.expectedErrMsg) + return + } + + require.False(t, res.IsError) + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedID, response.ID) + assert.Equal(t, tc.expectedURL, response.URL) + }) + } +} + +func Test_SetDiscussionCommentAnswer(t *testing.T) { + t.Parallel() + + toolDef := SetDiscussionCommentAnswer(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "set_discussion_comment_answer", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint, "set_discussion_comment_answer should not be read-only") + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Contains(t, schema.Properties, "commentNodeID") + assert.Contains(t, schema.Properties, "isAnswer") + assert.ElementsMatch(t, schema.Required, []string{"commentNodeID", "isAnswer"}) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedDiscussionID string + expectedDiscussionURL string + }{ + { + name: "successful mark as answer", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "isAnswer": true, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + MarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + }{}, + githubv4.MarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "markDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDODiscussion123", + "url": "https://github.com/owner/repo/discussions/1", + }, + }, + }), + ), + ), + expectToolError: false, + expectedDiscussionID: "D_kwDODiscussion123", + expectedDiscussionURL: "https://github.com/owner/repo/discussions/1", + }, + { + name: "successful unmark as answer", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "isAnswer": false, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UnmarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + }{}, + githubv4.UnmarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "unmarkDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDODiscussion123", + "url": "https://github.com/owner/repo/discussions/1", + }, + }, + }), + ), + ), + expectToolError: false, + expectedDiscussionID: "D_kwDODiscussion123", + expectedDiscussionURL: "https://github.com/owner/repo/discussions/1", + }, + { + name: "comment not in answerable category", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "isAnswer": true, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + MarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + }{}, + githubv4.MarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.ErrorResponse("Comment 'DC_kwDOComment456' does not belong to a discussion in a category that supports answers."), + ), + ), + expectToolError: true, + expectedErrMsg: "does not belong to a discussion in a category that supports answers", + }, + { + name: "unmark error from API", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + "isAnswer": false, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UnmarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + }{}, + githubv4.UnmarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOComment456'."), + ), + ), + expectToolError: true, + expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOComment456'.", + }, + { + name: "missing isAnswer param", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "isAnswer is required", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + req := createMCPRequest(tc.requestArgs) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + + if tc.expectToolError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.expectedErrMsg) + return + } + + require.False(t, res.IsError) + var response struct { + DiscussionID string `json:"discussionID"` + DiscussionURL string `json:"discussionURL"` + } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedDiscussionID, response.DiscussionID) + assert.Equal(t, tc.expectedDiscussionURL, response.DiscussionURL) + }) + } +} + +func Test_DeleteDiscussionComment(t *testing.T) { + t.Parallel() + + // Verify tool definition and schema + toolDef := DeleteDiscussionComment(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "delete_discussion_comment", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint, "delete_discussion_comment should not be read-only") + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Contains(t, schema.Properties, "commentNodeID") + assert.ElementsMatch(t, schema.Required, []string{"commentNodeID"}) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedID string + expectedURL string + }{ + { + name: "successful comment delete", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + }{}, + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "deleteDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + expectToolError: false, + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + { + name: "comment not found", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOInvalid", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + }{}, + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOInvalid"), + }, + nil, + githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOInvalid'."), + ), + ), + expectToolError: true, + expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOInvalid'.", + }, + { + name: "insufficient permissions", + requestArgs: map[string]any{ + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + }{}, + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, + githubv4mock.ErrorResponse("insufficient permissions to delete this discussion comment"), + ), + ), + expectToolError: true, + expectedErrMsg: "insufficient permissions to delete this discussion comment", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + req := createMCPRequest(tc.requestArgs) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + + if tc.expectToolError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.expectedErrMsg) + return + } + + require.False(t, res.IsError) + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedID, response.ID) + assert.Equal(t, tc.expectedURL, response.URL) + }) + } +} diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index a8757c51c3..2a0ef6a613 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -51,6 +51,15 @@ type MinimalSearchRepositoriesResult struct { Items []MinimalRepository `json:"items"` } +// MinimalDiscussionComment is the trimmed output type for discussion comment objects. +type MinimalDiscussionComment struct { + ID string `json:"id"` + Body string `json:"body"` + IsAnswer bool `json:"isAnswer,omitempty"` + Replies []MinimalDiscussionComment `json:"replies,omitempty"` + ReplyTotalCount int `json:"replyTotalCount,omitempty"` +} + // MinimalCommitAuthor represents commit author information. type MinimalCommitAuthor struct { Name string `json:"name,omitempty"` diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 559088f6d6..37e0e6a6ca 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -258,6 +258,10 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { ListDiscussions(t), GetDiscussion(t), GetDiscussionComments(t), + AddDiscussionComment(t), + UpdateDiscussionComment(t), + DeleteDiscussionComment(t), + SetDiscussionCommentAnswer(t), ListDiscussionCategories(t), // Actions tools From 192ca0b3a9b48dd3776f1880a14836258176c1da Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Tue, 5 May 2026 15:08:02 +0100 Subject: [PATCH 2/6] Address comments from Copilot review --- pkg/github/discussions.go | 51 +++++++++++++++++++--------------- pkg/github/discussions_test.go | 2 +- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 7f79354a4a..b05c43cc74 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -622,14 +622,20 @@ func AddDiscussionComment(t translations.TranslationHelperFunc) inventory.Server }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - // Decode params - var params struct { - Owner string - Repo string - DiscussionNumber int32 - Body string + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + discussionNumber, err := RequiredInt(args, "discussionNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, err := RequiredParam[string](args, "body") + if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetGQLClient(ctx) @@ -646,9 +652,9 @@ func AddDiscussionComment(t translations.TranslationHelperFunc) inventory.Server } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]any{ - "owner": githubv4.String(params.Owner), - "repo": githubv4.String(params.Repo), - "discussionNumber": githubv4.Int(params.DiscussionNumber), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers } if err := client.Query(ctx, &q, vars); err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -657,7 +663,7 @@ func AddDiscussionComment(t translations.TranslationHelperFunc) inventory.Server // Add the comment using the discussion's node ID input := githubv4.AddDiscussionCommentInput{ DiscussionID: q.Repository.Discussion.ID, - Body: githubv4.String(params.Body), + Body: githubv4.String(body), } replyToCommentNodeID, err := OptionalParam[string](args, "replyToCommentNodeID") @@ -745,11 +751,12 @@ func UpdateDiscussionComment(t translations.TranslationHelperFunc) inventory.Ser }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - var params struct { - CommentNodeID string - Body string + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + body, err := RequiredParam[string](args, "body") + if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetGQLClient(ctx) @@ -758,8 +765,8 @@ func UpdateDiscussionComment(t translations.TranslationHelperFunc) inventory.Ser } input := githubv4.UpdateDiscussionCommentInput{ - CommentID: githubv4.ID(params.CommentNodeID), - Body: githubv4.String(params.Body), + CommentID: githubv4.ID(commentNodeID), + Body: githubv4.String(body), } var mutation struct { @@ -813,10 +820,8 @@ func DeleteDiscussionComment(t translations.TranslationHelperFunc) inventory.Ser }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - var params struct { - CommentNodeID string - } - if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetGQLClient(ctx) @@ -825,7 +830,7 @@ func DeleteDiscussionComment(t translations.TranslationHelperFunc) inventory.Ser } input := githubv4.DeleteDiscussionCommentInput{ - ID: githubv4.ID(params.CommentNodeID), + ID: githubv4.ID(commentNodeID), } var mutation struct { @@ -888,7 +893,7 @@ func SetDiscussionCommentAnswer(t translations.TranslationHelperFunc) inventory. return utils.NewToolResultError(err.Error()), nil, nil } if _, ok := args["isAnswer"]; !ok { - return utils.NewToolResultError("isAnswer is required"), nil, nil + return utils.NewToolResultError("missing required parameter: isAnswer"), nil, nil } isAnswer, err := OptionalParam[bool](args, "isAnswer") if err != nil { diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index bb971b4da0..f6b7709675 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -1678,7 +1678,7 @@ func Test_SetDiscussionCommentAnswer(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient(), expectToolError: true, - expectedErrMsg: "isAnswer is required", + expectedErrMsg: "missing required parameter: isAnswer", }, } From cbd22dd93548ef3c1c80cac92201a28009f71dbb Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Tue, 5 May 2026 15:16:00 +0100 Subject: [PATCH 3/6] Update includeReplies description to specify GitHub API maximum replies limit --- README.md | 2 +- pkg/github/__toolsnaps__/get_discussion_comments.snap | 2 +- pkg/github/discussions.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8850a2ac4c..65cbe316ad 100644 --- a/README.md +++ b/README.md @@ -752,7 +752,7 @@ The following sets of tools are available: - **Required OAuth Scopes**: `repo` - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `discussionNumber`: Discussion Number (number, required) - - `includeReplies`: When true, each top-level comment will include its replies nested within it. Defaults to false. (boolean, optional) + - `includeReplies`: When true, each top-level comment will include its replies nested within it (up to 100 replies per comment, which is the GitHub API maximum). Defaults to false. (boolean, optional) - `owner`: Repository owner (string, required) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) diff --git a/pkg/github/__toolsnaps__/get_discussion_comments.snap b/pkg/github/__toolsnaps__/get_discussion_comments.snap index 12f15f2097..422fc40bf7 100644 --- a/pkg/github/__toolsnaps__/get_discussion_comments.snap +++ b/pkg/github/__toolsnaps__/get_discussion_comments.snap @@ -15,7 +15,7 @@ "type": "number" }, "includeReplies": { - "description": "When true, each top-level comment will include its replies nested within it. Defaults to false.", + "description": "When true, each top-level comment will include its replies nested within it (up to 100 replies per comment, which is the GitHub API maximum). Defaults to false.", "type": "boolean" }, "owner": { diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index b05c43cc74..b8a249ddd9 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -408,7 +408,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve }, "includeReplies": { Type: "boolean", - Description: "When true, each top-level comment will include its replies nested within it. Defaults to false.", + Description: "When true, each top-level comment will include its replies nested within it (up to 100 replies per comment, which is the GitHub API maximum). Defaults to false.", }, }, Required: []string{"owner", "repo", "discussionNumber"}, From f2a9bafe4665b92d92db90b50cf10558474705d8 Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Fri, 8 May 2026 10:43:34 +0100 Subject: [PATCH 4/6] Consolidate into single tool --- README.md | 35 +- .../__toolsnaps__/add_discussion_comment.snap | 38 - .../delete_discussion_comment.snap | 19 - .../discussion_comment_write.snap | 47 + .../set_discussion_comment_answer.snap | 24 - .../update_discussion_comment.snap | 24 - pkg/github/discussions.go | 623 ++++++------- pkg/github/discussions_test.go | 865 +++++++----------- pkg/github/tools.go | 5 +- 9 files changed, 709 insertions(+), 971 deletions(-) delete mode 100644 pkg/github/__toolsnaps__/add_discussion_comment.snap delete mode 100644 pkg/github/__toolsnaps__/delete_discussion_comment.snap create mode 100644 pkg/github/__toolsnaps__/discussion_comment_write.snap delete mode 100644 pkg/github/__toolsnaps__/set_discussion_comment_answer.snap delete mode 100644 pkg/github/__toolsnaps__/update_discussion_comment.snap diff --git a/README.md b/README.md index 65cbe316ad..999f81e7aa 100644 --- a/README.md +++ b/README.md @@ -730,17 +730,22 @@ The following sets of tools are available: comment-discussion Discussions -- **add_discussion_comment** - Add discussion comment +- **discussion_comment_write** - Manage discussion comments - **Required OAuth Scopes**: `repo` - - `body`: Comment content (string, required) - - `discussionNumber`: Discussion Number (number, required) - - `owner`: Repository owner (string, required) - - `replyToCommentNodeID`: The Node ID of the comment to reply to. If provided, the comment will be posted as a reply. (string, optional) - - `repo`: Repository name (string, required) - -- **delete_discussion_comment** - Delete discussion comment - - **Required OAuth Scopes**: `repo` - - `commentNodeID`: The Node ID of the discussion comment to delete (string, required) + - `body`: Comment content (required for 'add', 'reply', and 'update' methods) (string, optional) + - `commentNodeID`: The Node ID of the discussion comment (required for 'reply', 'update', 'delete', 'mark_answer', and 'unmark_answer' methods). For 'reply', this is the top-level comment to reply to; GitHub Discussions only support one level of nesting. (string, optional) + - `discussionNumber`: Discussion number (required for 'add' and 'reply' methods) (number, optional) + - `method`: Write operation to perform on a discussion comment. + Options are: + - 'add' - adds a new top-level comment to a discussion. + - 'reply' - replies to a top-level discussion comment (GitHub Discussions only support one level of nesting). + - 'update' - updates an existing discussion comment. + - 'delete' - deletes a discussion comment. + - 'mark_answer' - marks a discussion comment as the answer. + - 'unmark_answer' - unmarks a discussion comment as the answer. + (string, required) + - `owner`: Repository owner (required for 'add' and 'reply' methods) (string, optional) + - `repo`: Repository name (required for 'add' and 'reply' methods) (string, optional) - **get_discussion** - Get discussion - **Required OAuth Scopes**: `repo` @@ -772,16 +777,6 @@ The following sets of tools are available: - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name. If not provided, discussions will be queried at the organisation level. (string, optional) -- **set_discussion_comment_answer** - Set discussion comment as answer - - **Required OAuth Scopes**: `repo` - - `commentNodeID`: The Node ID of the discussion comment to mark or unmark as the answer (string, required) - - `isAnswer`: Whether the comment is the answer to the discussion (true to mark, false to unmark) (boolean, required) - -- **update_discussion_comment** - Update discussion comment - - **Required OAuth Scopes**: `repo` - - `body`: The new contents of the comment (string, required) - - `commentNodeID`: The Node ID of the discussion comment to update (string, required) -
diff --git a/pkg/github/__toolsnaps__/add_discussion_comment.snap b/pkg/github/__toolsnaps__/add_discussion_comment.snap deleted file mode 100644 index e2044edbe4..0000000000 --- a/pkg/github/__toolsnaps__/add_discussion_comment.snap +++ /dev/null @@ -1,38 +0,0 @@ -{ - "annotations": { - "title": "Add discussion comment" - }, - "description": "Add a comment to a discussion", - "inputSchema": { - "properties": { - "body": { - "description": "Comment content", - "type": "string" - }, - "discussionNumber": { - "description": "Discussion Number", - "type": "number" - }, - "owner": { - "description": "Repository owner", - "type": "string" - }, - "replyToCommentNodeID": { - "description": "The Node ID of the comment to reply to. If provided, the comment will be posted as a reply.", - "type": "string" - }, - "repo": { - "description": "Repository name", - "type": "string" - } - }, - "required": [ - "owner", - "repo", - "discussionNumber", - "body" - ], - "type": "object" - }, - "name": "add_discussion_comment" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/delete_discussion_comment.snap b/pkg/github/__toolsnaps__/delete_discussion_comment.snap deleted file mode 100644 index 43cee5d429..0000000000 --- a/pkg/github/__toolsnaps__/delete_discussion_comment.snap +++ /dev/null @@ -1,19 +0,0 @@ -{ - "annotations": { - "title": "Delete discussion comment" - }, - "description": "Delete a comment on a discussion", - "inputSchema": { - "properties": { - "commentNodeID": { - "description": "The Node ID of the discussion comment to delete", - "type": "string" - } - }, - "required": [ - "commentNodeID" - ], - "type": "object" - }, - "name": "delete_discussion_comment" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/discussion_comment_write.snap b/pkg/github/__toolsnaps__/discussion_comment_write.snap new file mode 100644 index 0000000000..b8d0129bd0 --- /dev/null +++ b/pkg/github/__toolsnaps__/discussion_comment_write.snap @@ -0,0 +1,47 @@ +{ + "annotations": { + "title": "Manage discussion comments" + }, + "description": "Write operations for discussion comments.\nSupports adding top-level comments, replying to existing comments, updating comment content, deleting comments, and marking or unmarking comments as the answer.", + "inputSchema": { + "properties": { + "body": { + "description": "Comment content (required for 'add', 'reply', and 'update' methods)", + "type": "string" + }, + "commentNodeID": { + "description": "The Node ID of the discussion comment (required for 'reply', 'update', 'delete', 'mark_answer', and 'unmark_answer' methods). For 'reply', this is the top-level comment to reply to; GitHub Discussions only support one level of nesting.", + "type": "string" + }, + "discussionNumber": { + "description": "Discussion number (required for 'add' and 'reply' methods)", + "type": "number" + }, + "method": { + "description": "Write operation to perform on a discussion comment.\nOptions are:\n- 'add' - adds a new top-level comment to a discussion.\n- 'reply' - replies to a top-level discussion comment (GitHub Discussions only support one level of nesting).\n- 'update' - updates an existing discussion comment.\n- 'delete' - deletes a discussion comment.\n- 'mark_answer' - marks a discussion comment as the answer.\n- 'unmark_answer' - unmarks a discussion comment as the answer.\n", + "enum": [ + "add", + "reply", + "update", + "delete", + "mark_answer", + "unmark_answer" + ], + "type": "string" + }, + "owner": { + "description": "Repository owner (required for 'add' and 'reply' methods)", + "type": "string" + }, + "repo": { + "description": "Repository name (required for 'add' and 'reply' methods)", + "type": "string" + } + }, + "required": [ + "method" + ], + "type": "object" + }, + "name": "discussion_comment_write" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap b/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap deleted file mode 100644 index a631ad305c..0000000000 --- a/pkg/github/__toolsnaps__/set_discussion_comment_answer.snap +++ /dev/null @@ -1,24 +0,0 @@ -{ - "annotations": { - "title": "Set discussion comment as answer" - }, - "description": "Marks or unmarks a given discussion comment as the answer to the discussion.", - "inputSchema": { - "properties": { - "commentNodeID": { - "description": "The Node ID of the discussion comment to mark or unmark as the answer", - "type": "string" - }, - "isAnswer": { - "description": "Whether the comment is the answer to the discussion (true to mark, false to unmark)", - "type": "boolean" - } - }, - "required": [ - "commentNodeID", - "isAnswer" - ], - "type": "object" - }, - "name": "set_discussion_comment_answer" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_discussion_comment.snap b/pkg/github/__toolsnaps__/update_discussion_comment.snap deleted file mode 100644 index 262f1b5710..0000000000 --- a/pkg/github/__toolsnaps__/update_discussion_comment.snap +++ /dev/null @@ -1,24 +0,0 @@ -{ - "annotations": { - "title": "Update discussion comment" - }, - "description": "Update a comment on a discussion", - "inputSchema": { - "properties": { - "body": { - "description": "The new contents of the comment", - "type": "string" - }, - "commentNodeID": { - "description": "The Node ID of the discussion comment to update", - "type": "string" - } - }, - "required": [ - "commentNodeID", - "body" - ], - "type": "object" - }, - "name": "update_discussion_comment" -} \ No newline at end of file diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index b8a249ddd9..4037a567d7 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -489,8 +489,9 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve IsAnswer githubv4.Boolean Replies struct { Nodes []struct { - ID githubv4.ID - Body githubv4.String + ID githubv4.ID + Body githubv4.String + IsAnswer githubv4.Boolean } TotalCount int } `graphql:"replies(first: 100)"` @@ -518,8 +519,9 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve } for _, r := range c.Replies.Nodes { comment.Replies = append(comment.Replies, MinimalDiscussionComment{ - ID: fmt.Sprintf("%v", r.ID), - Body: string(r.Body), + ID: fmt.Sprintf("%v", r.ID), + Body: string(r.Body), + IsAnswer: bool(r.IsAnswer), }) } comments = append(comments, comment) @@ -583,383 +585,390 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve ) } -func AddDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { +func DiscussionCommentWrite(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( ToolsetMetadataDiscussions, mcp.Tool{ - Name: "add_discussion_comment", - Description: t("TOOL_ADD_DISCUSSION_COMMENT_DESCRIPTION", "Add a comment to a discussion"), + Name: "discussion_comment_write", + Description: t("TOOL_DISCUSSION_COMMENT_WRITE_DESCRIPTION", `Write operations for discussion comments. +Supports adding top-level comments, replying to existing comments, updating comment content, deleting comments, and marking or unmarking comments as the answer.`), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_ADD_DISCUSSION_COMMENT_USER_TITLE", "Add discussion comment"), + Title: t("TOOL_DISCUSSION_COMMENT_WRITE_USER_TITLE", "Manage discussion comments"), ReadOnlyHint: false, }, InputSchema: &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ + "method": { + Type: "string", + Description: `Write operation to perform on a discussion comment. +Options are: +- 'add' - adds a new top-level comment to a discussion. +- 'reply' - replies to a top-level discussion comment (GitHub Discussions only support one level of nesting). +- 'update' - updates an existing discussion comment. +- 'delete' - deletes a discussion comment. +- 'mark_answer' - marks a discussion comment as the answer. +- 'unmark_answer' - unmarks a discussion comment as the answer. +`, + Enum: []any{"add", "reply", "update", "delete", "mark_answer", "unmark_answer"}, + }, "owner": { Type: "string", - Description: "Repository owner", + Description: "Repository owner (required for 'add' and 'reply' methods)", }, "repo": { Type: "string", - Description: "Repository name", + Description: "Repository name (required for 'add' and 'reply' methods)", }, "discussionNumber": { Type: "number", - Description: "Discussion Number", + Description: "Discussion number (required for 'add' and 'reply' methods)", }, "body": { Type: "string", - Description: "Comment content", + Description: "Comment content (required for 'add', 'reply', and 'update' methods)", }, - "replyToCommentNodeID": { + "commentNodeID": { Type: "string", - Description: "The Node ID of the comment to reply to. If provided, the comment will be posted as a reply.", + Description: "The Node ID of the discussion comment (required for 'reply', 'update', 'delete', 'mark_answer', and 'unmark_answer' methods). For 'reply', this is the top-level comment to reply to; GitHub Discussions only support one level of nesting.", }, }, - Required: []string{"owner", "repo", "discussionNumber", "body"}, + Required: []string{"method"}, }, }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - owner, err := RequiredParam[string](args, "owner") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - repo, err := RequiredParam[string](args, "repo") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - discussionNumber, err := RequiredInt(args, "discussionNumber") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - body, err := RequiredParam[string](args, "body") + method, err := RequiredParam[string](args, "method") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + client, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil } - // Get the discussion's node ID using its number - var q struct { - Repository struct { - Discussion struct { - ID githubv4.ID - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - vars := map[string]any{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers - } - if err := client.Query(ctx, &q, vars); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + switch method { + case "add": + return addDiscussionComment(ctx, client, args) + case "reply": + return replyToDiscussionComment(ctx, client, args) + case "update": + return updateDiscussionComment(ctx, client, args) + case "delete": + return deleteDiscussionComment(ctx, client, args) + case "mark_answer": + return markDiscussionCommentAsAnswer(ctx, client, args) + case "unmark_answer": + return unmarkDiscussionCommentAsAnswer(ctx, client, args) + default: + return utils.NewToolResultError("invalid method, must be one of: 'add', 'reply', 'update', 'delete', 'mark_answer', 'unmark_answer'"), nil, nil } + }) +} - // Add the comment using the discussion's node ID - input := githubv4.AddDiscussionCommentInput{ - DiscussionID: q.Repository.Discussion.ID, - Body: githubv4.String(body), - } +func addDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + discussionNumber, err := RequiredInt(args, "discussionNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, err := RequiredParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - replyToCommentNodeID, err := OptionalParam[string](args, "replyToCommentNodeID") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - if replyToCommentNodeID != "" { - if strings.TrimSpace(replyToCommentNodeID) == "" { - return utils.NewToolResultError("replyToCommentNodeID cannot be blank"), nil, nil - } - // The GitHub API silently ignores an invalid ReplyToID and creates a top-level - // comment instead of returning an error, so we validate upfront that the node - // exists and is a DiscussionComment to give callers a clear failure. - var nodeQuery struct { - Node struct { - DiscussionComment struct { - ID githubv4.ID - } `graphql:"... on DiscussionComment"` - } `graphql:"node(id: $replyToID)"` - } - if err := client.Query(ctx, &nodeQuery, map[string]any{ - "replyToID": githubv4.ID(replyToCommentNodeID), - }); err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to validate replyToCommentNodeID: %v", err)), nil, nil - } - if nodeQuery.Node.DiscussionComment.ID == nil || nodeQuery.Node.DiscussionComment.ID == "" { - return utils.NewToolResultError(fmt.Sprintf("replyToCommentNodeID %q does not resolve to a valid discussion comment", replyToCommentNodeID)), nil, nil - } - id := githubv4.ID(replyToCommentNodeID) - input.ReplyToID = &id - } + // Get the discussion's node ID using its number + var q struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + vars := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers + } + if err := client.Query(ctx, &q, vars); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - var mutation struct { - AddDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"addDiscussionComment(input: $input)"` - } + input := githubv4.AddDiscussionCommentInput{ + DiscussionID: q.Repository.Discussion.ID, + Body: githubv4.String(body), + } - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + var mutation struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"addDiscussionComment(input: $input)"` + } - comment := mutation.AddDiscussionComment.Comment - minimalResponse := MinimalResponse{ - ID: fmt.Sprintf("%v", comment.ID), - URL: string(comment.URL), - } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - out, err := json.Marshal(minimalResponse) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) - } + comment := mutation.AddDiscussionComment.Comment + out, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } - return utils.NewToolResultText(string(out)), nil, nil - }) + return utils.NewToolResultText(string(out)), nil, nil } -func UpdateDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( - ToolsetMetadataDiscussions, - mcp.Tool{ - Name: "update_discussion_comment", - Description: t("TOOL_UPDATE_DISCUSSION_COMMENT_DESCRIPTION", "Update a comment on a discussion"), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_UPDATE_DISCUSSION_COMMENT_USER_TITLE", "Update discussion comment"), - ReadOnlyHint: false, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "commentNodeID": { - Type: "string", - Description: "The Node ID of the discussion comment to update", - }, - "body": { - Type: "string", - Description: "The new contents of the comment", - }, - }, - Required: []string{"commentNodeID", "body"}, - }, - }, - []scopes.Scope{scopes.Repo}, - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - body, err := RequiredParam[string](args, "body") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - client, err := deps.GetGQLClient(ctx) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil - } +func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if strings.TrimSpace(commentNodeID) == "" { + return utils.NewToolResultError("commentNodeID cannot be blank"), nil, nil + } - input := githubv4.UpdateDiscussionCommentInput{ - CommentID: githubv4.ID(commentNodeID), - Body: githubv4.String(body), - } + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + discussionNumber, err := RequiredInt(args, "discussionNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, err := RequiredParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - var mutation struct { - UpdateDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"updateDiscussionComment(input: $input)"` - } + // The GitHub API silently ignores an invalid ReplyToID and creates a top-level + // comment instead of returning an error, so we validate upfront that the node + // exists and is a DiscussionComment to give callers a clear failure. + var nodeQuery struct { + Node struct { + DiscussionComment struct { + ID githubv4.ID + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + } + if err := client.Query(ctx, &nodeQuery, map[string]any{ + "replyToID": githubv4.ID(commentNodeID), + }); err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to validate commentNodeID: %v", err)), nil, nil + } + if nodeQuery.Node.DiscussionComment.ID == nil || nodeQuery.Node.DiscussionComment.ID == "" { + return utils.NewToolResultError(fmt.Sprintf("commentNodeID %q does not resolve to a valid discussion comment", commentNodeID)), nil, nil + } - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + // Get the discussion's node ID using its number + var q struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + vars := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers + } + if err := client.Query(ctx, &q, vars); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - comment := mutation.UpdateDiscussionComment.Comment - minimalResponse := MinimalResponse{ - ID: fmt.Sprintf("%v", comment.ID), - URL: string(comment.URL), - } + replyToID := githubv4.ID(commentNodeID) + input := githubv4.AddDiscussionCommentInput{ + DiscussionID: q.Repository.Discussion.ID, + Body: githubv4.String(body), + ReplyToID: &replyToID, + } - out, err := json.Marshal(minimalResponse) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + var mutation struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"addDiscussionComment(input: $input)"` + } - return utils.NewToolResultText(string(out)), nil, nil - }) + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + comment := mutation.AddDiscussionComment.Comment + out, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil } -func DeleteDiscussionComment(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( - ToolsetMetadataDiscussions, - mcp.Tool{ - Name: "delete_discussion_comment", - Description: t("TOOL_DELETE_DISCUSSION_COMMENT_DESCRIPTION", "Delete a comment on a discussion"), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_DELETE_DISCUSSION_COMMENT_USER_TITLE", "Delete discussion comment"), - ReadOnlyHint: false, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "commentNodeID": { - Type: "string", - Description: "The Node ID of the discussion comment to delete", - }, - }, - Required: []string{"commentNodeID"}, - }, - }, - []scopes.Scope{scopes.Repo}, - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - client, err := deps.GetGQLClient(ctx) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil - } +func updateDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, err := RequiredParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - input := githubv4.DeleteDiscussionCommentInput{ - ID: githubv4.ID(commentNodeID), - } + input := githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID(commentNodeID), + Body: githubv4.String(body), + } - var mutation struct { - DeleteDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"deleteDiscussionComment(input: $input)"` + var mutation struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"updateDiscussionComment(input: $input)"` + } - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - comment := mutation.DeleteDiscussionComment.Comment - minimalResponse := MinimalResponse{ - ID: fmt.Sprintf("%v", comment.ID), - URL: string(comment.URL), - } + comment := mutation.UpdateDiscussionComment.Comment + out, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } - out, err := json.Marshal(minimalResponse) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + return utils.NewToolResultText(string(out)), nil, nil +} + +func deleteDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + input := githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID(commentNodeID), + } + + var mutation struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"deleteDiscussionComment(input: $input)"` + } - return utils.NewToolResultText(string(out)), nil, nil - }) + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + comment := mutation.DeleteDiscussionComment.Comment + out, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%v", comment.ID), + URL: string(comment.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal comment: %w", err) + } + + return utils.NewToolResultText(string(out)), nil, nil } -func SetDiscussionCommentAnswer(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( - ToolsetMetadataDiscussions, - mcp.Tool{ - Name: "set_discussion_comment_answer", - Description: t("TOOL_SET_DISCUSSION_COMMENT_ANSWER_DESCRIPTION", "Marks or unmarks a given discussion comment as the answer to the discussion."), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_SET_DISCUSSION_COMMENT_ANSWER_USER_TITLE", "Set discussion comment as answer"), - ReadOnlyHint: false, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "commentNodeID": { - Type: "string", - Description: "The Node ID of the discussion comment to mark or unmark as the answer", - }, - "isAnswer": { - Type: "boolean", - Description: "Whether the comment is the answer to the discussion (true to mark, false to unmark)", - }, - }, - Required: []string{"commentNodeID", "isAnswer"}, - }, - }, - []scopes.Scope{scopes.Repo}, - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - if _, ok := args["isAnswer"]; !ok { - return utils.NewToolResultError("missing required parameter: isAnswer"), nil, nil - } - isAnswer, err := OptionalParam[bool](args, "isAnswer") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } +func markDiscussionCommentAsAnswer(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - client, err := deps.GetGQLClient(ctx) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + input := githubv4.MarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID(commentNodeID), + } + var mutation struct { + MarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + out, err := json.Marshal(struct { + DiscussionID string `json:"discussionID"` + DiscussionURL string `json:"discussionURL"` + }{ + DiscussionID: fmt.Sprintf("%v", mutation.MarkDiscussionCommentAsAnswer.Discussion.ID), + DiscussionURL: string(mutation.MarkDiscussionCommentAsAnswer.Discussion.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal discussion: %w", err) + } - var discussionID githubv4.ID - var discussionURL githubv4.String + return utils.NewToolResultText(string(out)), nil, nil +} - if isAnswer { - input := githubv4.MarkDiscussionCommentAsAnswerInput{ - ID: githubv4.ID(commentNodeID), - } - var mutation struct { - MarkDiscussionCommentAsAnswer struct { - Discussion struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` - } - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - discussionID = mutation.MarkDiscussionCommentAsAnswer.Discussion.ID - discussionURL = mutation.MarkDiscussionCommentAsAnswer.Discussion.URL - } else { - input := githubv4.UnmarkDiscussionCommentAsAnswerInput{ - ID: githubv4.ID(commentNodeID), - } - var mutation struct { - UnmarkDiscussionCommentAsAnswer struct { - Discussion struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` - } - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - discussionID = mutation.UnmarkDiscussionCommentAsAnswer.Discussion.ID - discussionURL = mutation.UnmarkDiscussionCommentAsAnswer.Discussion.URL - } +func unmarkDiscussionCommentAsAnswer(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := RequiredParam[string](args, "commentNodeID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - response := struct { - DiscussionID string `json:"discussionID"` - DiscussionURL string `json:"discussionURL"` - }{ - DiscussionID: fmt.Sprintf("%v", discussionID), - DiscussionURL: string(discussionURL), + input := githubv4.UnmarkDiscussionCommentAsAnswerInput{ + ID: githubv4.ID(commentNodeID), + } + var mutation struct { + UnmarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` } + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } - out, err := json.Marshal(response) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal discussion: %w", err) - } + out, err := json.Marshal(struct { + DiscussionID string `json:"discussionID"` + DiscussionURL string `json:"discussionURL"` + }{ + DiscussionID: fmt.Sprintf("%v", mutation.UnmarkDiscussionCommentAsAnswer.Discussion.ID), + DiscussionURL: string(mutation.UnmarkDiscussionCommentAsAnswer.Discussion.URL), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal discussion: %w", err) + } - return utils.NewToolResultText(string(out)), nil, nil - }, - ) + return utils.NewToolResultText(string(out)), nil, nil } func ListDiscussionCategories(t translations.TranslationHelperFunc) inventory.ServerTool { diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index f6b7709675..765c88c96f 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -930,65 +930,73 @@ func Test_ListDiscussionCategories(t *testing.T) { } } -func Test_AddDiscussionComment(t *testing.T) { +func Test_DiscussionCommentWrite(t *testing.T) { t.Parallel() // Verify tool definition and schema - toolDef := AddDiscussionComment(translations.NullTranslationHelper) + toolDef := DiscussionCommentWrite(translations.NullTranslationHelper) tool := toolDef.Tool require.NoError(t, toolsnaps.Test(tool.Name, tool)) - assert.Equal(t, "add_discussion_comment", tool.Name) + assert.Equal(t, "discussion_comment_write", tool.Name) assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint, "add_discussion_comment should not be read-only") + assert.False(t, tool.Annotations.ReadOnlyHint, "discussion_comment_write should not be read-only") schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Contains(t, schema.Properties, "method") assert.Contains(t, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "discussionNumber") assert.Contains(t, schema.Properties, "body") - assert.Contains(t, schema.Properties, "replyToCommentNodeID") - assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "discussionNumber", "body"}) + assert.Contains(t, schema.Properties, "commentNodeID") + assert.NotContains(t, schema.Properties, "replyToCommentNodeID") + assert.ElementsMatch(t, schema.Required, []string{"method"}) + + discussionQueryMatcher := githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Discussion struct { + ID githubv4.ID + } `graphql:"discussion(number: $discussionNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "discussionNumber": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + }, + }, + }), + ) tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedErrMsg string - expectedID string - expectedURL string + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedErrMsg string + expectedID string + expectedURL string + expectedDiscussionID string + expectedDiscussionURL string }{ + // add method tests { - name: "successful comment creation", + name: "add: successful comment creation", requestArgs: map[string]any{ + "method": "add", "owner": "owner", "repo": "repo", "discussionNumber": int32(1), "body": "This is a test comment", }, mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Discussion struct { - ID githubv4.ID - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "discussionNumber": githubv4.Int(1), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussion": map[string]any{ - "id": "D_kwDOTest123", - }, - }, - }), - ), + discussionQueryMatcher, githubv4mock.NewMutationMatcher( struct { AddDiscussionComment struct { @@ -1013,13 +1021,13 @@ func Test_AddDiscussionComment(t *testing.T) { }), ), ), - expectToolError: false, - expectedID: "DC_kwDOComment456", - expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", }, { - name: "discussion not found", + name: "add: discussion not found", requestArgs: map[string]any{ + "method": "add", "owner": "owner", "repo": "repo", "discussionNumber": int32(999), @@ -1046,35 +1054,16 @@ func Test_AddDiscussionComment(t *testing.T) { expectedErrMsg: "Could not resolve to a Discussion with the number of 999.", }, { - name: "mutation failure", + name: "add: mutation failure", requestArgs: map[string]any{ + "method": "add", "owner": "owner", "repo": "repo", "discussionNumber": int32(1), "body": "This is a comment", }, mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Discussion struct { - ID githubv4.ID - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "discussionNumber": githubv4.Int(1), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussion": map[string]any{ - "id": "D_kwDOTest123", - }, - }, - }), - ), + discussionQueryMatcher, githubv4mock.NewMutationMatcher( struct { AddDiscussionComment struct { @@ -1095,81 +1084,18 @@ func Test_AddDiscussionComment(t *testing.T) { expectToolError: true, expectedErrMsg: "insufficient permissions to comment on this discussion", }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(tc.mockedClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - req := createMCPRequest(tc.requestArgs) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) - - text := getTextResult(t, res).Text - - if tc.expectToolError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.expectedErrMsg) - return - } - - require.False(t, res.IsError) - var response MinimalResponse - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Equal(t, tc.expectedID, response.ID) - assert.Equal(t, tc.expectedURL, response.URL) - }) - } -} - -func Test_AddDiscussionCommentReply(t *testing.T) { - t.Parallel() - - toolDef := AddDiscussionComment(translations.NullTranslationHelper) - - queryMatcher := githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Discussion struct { - ID githubv4.ID - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "discussionNumber": githubv4.Int(1), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussion": map[string]any{ - "id": "D_kwDOTest123", - }, - }, - }), - ) - - tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedErrMsg string - expectedID string - expectedURL string - }{ + // reply method tests { - name: "successful reply to comment", + name: "reply: successful reply to comment", requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": int32(1), - "body": "This is a reply", - "replyToCommentNodeID": "DC_kwDOComment456", + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", }, mockedClient: githubv4mock.NewMockedHTTPClient( - queryMatcher, githubv4mock.NewQueryMatcher( struct { Node struct { @@ -1187,6 +1113,7 @@ func Test_AddDiscussionCommentReply(t *testing.T) { }, }), ), + discussionQueryMatcher, githubv4mock.NewMutationMatcher( struct { AddDiscussionComment struct { @@ -1212,34 +1139,47 @@ func Test_AddDiscussionCommentReply(t *testing.T) { }), ), ), - expectToolError: false, - expectedID: "DC_kwDOReply789", - expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-789", + expectedID: "DC_kwDOReply789", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-789", + }, + { + name: "reply: missing commentNodeID", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: commentNodeID", }, { - name: "whitespace-only replyToCommentNodeID is rejected", + name: "reply: whitespace-only commentNodeID is rejected", requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": int32(1), - "body": "This is a reply", - "replyToCommentNodeID": " ", + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": " ", }, - mockedClient: githubv4mock.NewMockedHTTPClient(queryMatcher), + mockedClient: githubv4mock.NewMockedHTTPClient(), expectToolError: true, - expectedErrMsg: "replyToCommentNodeID cannot be blank", + expectedErrMsg: "commentNodeID cannot be blank", }, { - name: "invalid replyToCommentNodeID returns error", + name: "reply: invalid commentNodeID returns error", requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": int32(1), - "body": "This is a reply", - "replyToCommentNodeID": "DC_kwDOInvalid", + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOInvalid", }, mockedClient: githubv4mock.NewMockedHTTPClient( - queryMatcher, githubv4mock.NewQueryMatcher( struct { Node struct { @@ -1257,161 +1197,76 @@ func Test_AddDiscussionCommentReply(t *testing.T) { ), ), expectToolError: true, - expectedErrMsg: `replyToCommentNodeID "DC_kwDOInvalid" does not resolve to a valid discussion comment`, + expectedErrMsg: `commentNodeID "DC_kwDOInvalid" does not resolve to a valid discussion comment`, }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(tc.mockedClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - req := createMCPRequest(tc.requestArgs) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) - - text := getTextResult(t, res).Text - - if tc.expectToolError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.expectedErrMsg) - return - } - - require.False(t, res.IsError) - var response MinimalResponse - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Equal(t, tc.expectedID, response.ID) - assert.Equal(t, tc.expectedURL, response.URL) - }) - } -} - -func githubv4ptr(id githubv4.ID) *githubv4.ID { - return &id -} - -func Test_GetDiscussionCommentsWithReplies(t *testing.T) { - t.Parallel() - - toolDef := GetDiscussionComments(translations.NullTranslationHelper) - - qWithReplies := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer,replies(first: 100){nodes{id,body},totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" - - vars := map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": float64(1), - "first": float64(30), - "after": (*string)(nil), - } - - mockResponse := githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussion": map[string]any{ - "comments": map[string]any{ - "nodes": []map[string]any{ - { - "id": "DC_id1", - "body": "Top-level comment", - "replies": map[string]any{ - "nodes": []map[string]any{ - {"id": "DC_reply1", "body": "Reply to first comment"}, - }, - "totalCount": 1, - }, - }, - { - "id": "DC_id2", - "body": "Another top-level comment", - "replies": map[string]any{ - "nodes": []map[string]any{}, - "totalCount": 0, + // update method tests + { + name: "update: successful comment update", + requestArgs: map[string]any{ + "method": "update", + "commentNodeID": "DC_kwDOComment456", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOComment456"), + Body: githubv4.String("Updated comment text"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", }, }, - }, - "pageInfo": map[string]any{ - "hasNextPage": false, - "hasPreviousPage": false, - "startCursor": "", - "endCursor": "", - }, - "totalCount": 2, - }, - }, + }), + ), + ), + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", }, - }) - - matcher := githubv4mock.NewQueryMatcher(qWithReplies, vars, mockResponse) - httpClient := githubv4mock.NewMockedHTTPClient(matcher) - gqlClient := githubv4.NewClient(httpClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - reqParams := map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": int32(1), - "includeReplies": true, - } - req := createMCPRequest(reqParams) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) - - text := getTextResult(t, res).Text - require.False(t, res.IsError, "expected no error, got: %s", text) - - var response struct { - Comments []MinimalDiscussionComment `json:"comments"` - PageInfo struct { - HasNextPage bool `json:"hasNextPage"` - } `json:"pageInfo"` - TotalCount int `json:"totalCount"` - } - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Len(t, response.Comments, 2) - - assert.Equal(t, "DC_id1", response.Comments[0].ID) - assert.Equal(t, "Top-level comment", response.Comments[0].Body) - require.Len(t, response.Comments[0].Replies, 1) - assert.Equal(t, "DC_reply1", response.Comments[0].Replies[0].ID) - assert.Equal(t, "Reply to first comment", response.Comments[0].Replies[0].Body) - assert.Equal(t, 1, response.Comments[0].ReplyTotalCount) - - assert.Equal(t, "DC_id2", response.Comments[1].ID) - assert.Empty(t, response.Comments[1].Replies) - assert.Equal(t, 0, response.Comments[1].ReplyTotalCount) -} - -func Test_UpdateDiscussionComment(t *testing.T) { - t.Parallel() - - // Verify tool definition and schema - toolDef := UpdateDiscussionComment(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - assert.Equal(t, "update_discussion_comment", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint, "update_discussion_comment should not be read-only") - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.Contains(t, schema.Properties, "commentNodeID") - assert.Contains(t, schema.Properties, "body") - assert.ElementsMatch(t, schema.Required, []string{"commentNodeID", "body"}) - - tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedErrMsg string - expectedID string - expectedURL string - }{ { - name: "successful comment update", + name: "update: comment not found", requestArgs: map[string]any{ + "method": "update", + "commentNodeID": "DC_kwDOInvalid", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOInvalid"), + Body: githubv4.String("Updated comment text"), + }, + nil, + githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOInvalid'."), + ), + ), + expectToolError: true, + expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOInvalid'.", + }, + { + name: "update: insufficient permissions", + requestArgs: map[string]any{ + "method": "update", "commentNodeID": "DC_kwDOComment456", "body": "Updated comment text", }, @@ -1430,8 +1285,35 @@ func Test_UpdateDiscussionComment(t *testing.T) { Body: githubv4.String("Updated comment text"), }, nil, + githubv4mock.ErrorResponse("insufficient permissions to update this discussion comment"), + ), + ), + expectToolError: true, + expectedErrMsg: "insufficient permissions to update this discussion comment", + }, + // delete method tests + { + name: "delete: successful comment delete", + requestArgs: map[string]any{ + "method": "delete", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + }{}, + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOComment456"), + }, + nil, githubv4mock.DataResponse(map[string]any{ - "updateDiscussionComment": map[string]any{ + "deleteDiscussionComment": map[string]any{ "comment": map[string]any{ "id": "DC_kwDOComment456", "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", @@ -1440,29 +1322,27 @@ func Test_UpdateDiscussionComment(t *testing.T) { }), ), ), - expectToolError: false, - expectedID: "DC_kwDOComment456", - expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", + expectedID: "DC_kwDOComment456", + expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", }, { - name: "comment not found", + name: "delete: comment not found", requestArgs: map[string]any{ + "method": "delete", "commentNodeID": "DC_kwDOInvalid", - "body": "Updated comment text", }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( struct { - UpdateDiscussionComment struct { + DeleteDiscussionComment struct { Comment struct { ID githubv4.ID URL githubv4.String `graphql:"url"` } - } `graphql:"updateDiscussionComment(input: $input)"` + } `graphql:"deleteDiscussionComment(input: $input)"` }{}, - githubv4.UpdateDiscussionCommentInput{ - CommentID: githubv4.ID("DC_kwDOInvalid"), - Body: githubv4.String("Updated comment text"), + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOInvalid"), }, nil, githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOInvalid'."), @@ -1472,90 +1352,37 @@ func Test_UpdateDiscussionComment(t *testing.T) { expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOInvalid'.", }, { - name: "insufficient permissions", + name: "delete: insufficient permissions", requestArgs: map[string]any{ + "method": "delete", "commentNodeID": "DC_kwDOComment456", - "body": "Updated comment text", }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( struct { - UpdateDiscussionComment struct { + DeleteDiscussionComment struct { Comment struct { ID githubv4.ID URL githubv4.String `graphql:"url"` } - } `graphql:"updateDiscussionComment(input: $input)"` + } `graphql:"deleteDiscussionComment(input: $input)"` }{}, - githubv4.UpdateDiscussionCommentInput{ - CommentID: githubv4.ID("DC_kwDOComment456"), - Body: githubv4.String("Updated comment text"), + githubv4.DeleteDiscussionCommentInput{ + ID: githubv4.ID("DC_kwDOComment456"), }, nil, - githubv4mock.ErrorResponse("insufficient permissions to update this discussion comment"), + githubv4mock.ErrorResponse("insufficient permissions to delete this discussion comment"), ), ), expectToolError: true, - expectedErrMsg: "insufficient permissions to update this discussion comment", + expectedErrMsg: "insufficient permissions to delete this discussion comment", }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(tc.mockedClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - req := createMCPRequest(tc.requestArgs) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) - - text := getTextResult(t, res).Text - - if tc.expectToolError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.expectedErrMsg) - return - } - - require.False(t, res.IsError) - var response MinimalResponse - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Equal(t, tc.expectedID, response.ID) - assert.Equal(t, tc.expectedURL, response.URL) - }) - } -} - -func Test_SetDiscussionCommentAnswer(t *testing.T) { - t.Parallel() - - toolDef := SetDiscussionCommentAnswer(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - assert.Equal(t, "set_discussion_comment_answer", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint, "set_discussion_comment_answer should not be read-only") - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.Contains(t, schema.Properties, "commentNodeID") - assert.Contains(t, schema.Properties, "isAnswer") - assert.ElementsMatch(t, schema.Required, []string{"commentNodeID", "isAnswer"}) - - tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedErrMsg string - expectedDiscussionID string - expectedDiscussionURL string - }{ + // mark_answer method tests { - name: "successful mark as answer", + name: "mark_answer: successful mark as answer", requestArgs: map[string]any{ + "method": "mark_answer", "commentNodeID": "DC_kwDOComment456", - "isAnswer": true, }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( @@ -1574,82 +1401,81 @@ func Test_SetDiscussionCommentAnswer(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "markDiscussionCommentAsAnswer": map[string]any{ "discussion": map[string]any{ - "id": "D_kwDODiscussion123", + "id": "D_kwDOTest123", "url": "https://github.com/owner/repo/discussions/1", }, }, }), ), ), - expectToolError: false, - expectedDiscussionID: "D_kwDODiscussion123", + expectedDiscussionID: "D_kwDOTest123", expectedDiscussionURL: "https://github.com/owner/repo/discussions/1", }, { - name: "successful unmark as answer", + name: "mark_answer: mutation failure", requestArgs: map[string]any{ + "method": "mark_answer", "commentNodeID": "DC_kwDOComment456", - "isAnswer": false, }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( struct { - UnmarkDiscussionCommentAsAnswer struct { + MarkDiscussionCommentAsAnswer struct { Discussion struct { ID githubv4.ID URL githubv4.String `graphql:"url"` } - } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` }{}, - githubv4.UnmarkDiscussionCommentAsAnswerInput{ + githubv4.MarkDiscussionCommentAsAnswerInput{ ID: githubv4.ID("DC_kwDOComment456"), }, nil, - githubv4mock.DataResponse(map[string]any{ - "unmarkDiscussionCommentAsAnswer": map[string]any{ - "discussion": map[string]any{ - "id": "D_kwDODiscussion123", - "url": "https://github.com/owner/repo/discussions/1", - }, - }, - }), + githubv4mock.ErrorResponse("discussion is not a Q&A discussion"), ), ), - expectToolError: false, - expectedDiscussionID: "D_kwDODiscussion123", - expectedDiscussionURL: "https://github.com/owner/repo/discussions/1", + expectToolError: true, + expectedErrMsg: "discussion is not a Q&A discussion", }, + // unmark_answer method tests { - name: "comment not in answerable category", + name: "unmark_answer: successful unmark as answer", requestArgs: map[string]any{ + "method": "unmark_answer", "commentNodeID": "DC_kwDOComment456", - "isAnswer": true, }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( struct { - MarkDiscussionCommentAsAnswer struct { + UnmarkDiscussionCommentAsAnswer struct { Discussion struct { ID githubv4.ID URL githubv4.String `graphql:"url"` } - } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` }{}, - githubv4.MarkDiscussionCommentAsAnswerInput{ + githubv4.UnmarkDiscussionCommentAsAnswerInput{ ID: githubv4.ID("DC_kwDOComment456"), }, nil, - githubv4mock.ErrorResponse("Comment 'DC_kwDOComment456' does not belong to a discussion in a category that supports answers."), + githubv4mock.DataResponse(map[string]any{ + "unmarkDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + "url": "https://github.com/owner/repo/discussions/1", + }, + }, + }), ), ), - expectToolError: true, - expectedErrMsg: "does not belong to a discussion in a category that supports answers", + expectedDiscussionID: "D_kwDOTest123", + expectedDiscussionURL: "https://github.com/owner/repo/discussions/1", }, { - name: "unmark error from API", + name: "unmark_answer: mutation failure", requestArgs: map[string]any{ + "method": "unmark_answer", "commentNodeID": "DC_kwDOComment456", - "isAnswer": false, }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewMutationMatcher( @@ -1665,23 +1491,23 @@ func Test_SetDiscussionCommentAnswer(t *testing.T) { ID: githubv4.ID("DC_kwDOComment456"), }, nil, - githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOComment456'."), + githubv4mock.ErrorResponse("insufficient permissions"), ), ), expectToolError: true, - expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOComment456'.", + expectedErrMsg: "insufficient permissions", }, + // invalid method test { - name: "missing isAnswer param", + name: "invalid method", requestArgs: map[string]any{ - "commentNodeID": "DC_kwDOComment456", + "method": "invalid", }, mockedClient: githubv4mock.NewMockedHTTPClient(), expectToolError: true, - expectedErrMsg: "missing required parameter: isAnswer", + expectedErrMsg: "invalid method, must be one of: 'add', 'reply', 'update', 'delete', 'mark_answer', 'unmark_answer'", }, } - for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { gqlClient := githubv4.NewClient(tc.mockedClient) @@ -1701,149 +1527,118 @@ func Test_SetDiscussionCommentAnswer(t *testing.T) { } require.False(t, res.IsError) - var response struct { - DiscussionID string `json:"discussionID"` - DiscussionURL string `json:"discussionURL"` + + if tc.expectedDiscussionID != "" { + var response struct { + DiscussionID string `json:"discussionID"` + DiscussionURL string `json:"discussionURL"` + } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedDiscussionID, response.DiscussionID) + assert.Equal(t, tc.expectedDiscussionURL, response.DiscussionURL) + } else { + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Equal(t, tc.expectedID, response.ID) + assert.Equal(t, tc.expectedURL, response.URL) } - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Equal(t, tc.expectedDiscussionID, response.DiscussionID) - assert.Equal(t, tc.expectedDiscussionURL, response.DiscussionURL) }) } } -func Test_DeleteDiscussionComment(t *testing.T) { +func githubv4ptr(id githubv4.ID) *githubv4.ID { + return &id +} + +func Test_GetDiscussionCommentsWithReplies(t *testing.T) { t.Parallel() - // Verify tool definition and schema - toolDef := DeleteDiscussionComment(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + toolDef := GetDiscussionComments(translations.NullTranslationHelper) - assert.Equal(t, "delete_discussion_comment", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint, "delete_discussion_comment should not be read-only") - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.Contains(t, schema.Properties, "commentNodeID") - assert.ElementsMatch(t, schema.Required, []string{"commentNodeID"}) + qWithReplies := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer,replies(first: 100){nodes{id,body,isAnswer},totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" - tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedErrMsg string - expectedID string - expectedURL string - }{ - { - name: "successful comment delete", - requestArgs: map[string]any{ - "commentNodeID": "DC_kwDOComment456", - }, - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewMutationMatcher( - struct { - DeleteDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"deleteDiscussionComment(input: $input)"` - }{}, - githubv4.DeleteDiscussionCommentInput{ - ID: githubv4.ID("DC_kwDOComment456"), - }, - nil, - githubv4mock.DataResponse(map[string]any{ - "deleteDiscussionComment": map[string]any{ - "comment": map[string]any{ - "id": "DC_kwDOComment456", - "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + "first": float64(30), + "after": (*string)(nil), + } + + mockResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "comments": map[string]any{ + "nodes": []map[string]any{ + { + "id": "DC_id1", + "body": "Top-level comment", + "replies": map[string]any{ + "nodes": []map[string]any{ + {"id": "DC_reply1", "body": "Reply to first comment", "isAnswer": true}, + }, + "totalCount": 1, + }, + }, + { + "id": "DC_id2", + "body": "Another top-level comment", + "replies": map[string]any{ + "nodes": []map[string]any{}, + "totalCount": 0, }, }, - }), - ), - ), - expectToolError: false, - expectedID: "DC_kwDOComment456", - expectedURL: "https://github.com/owner/repo/discussions/1#discussioncomment-456", - }, - { - name: "comment not found", - requestArgs: map[string]any{ - "commentNodeID": "DC_kwDOInvalid", - }, - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewMutationMatcher( - struct { - DeleteDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"deleteDiscussionComment(input: $input)"` - }{}, - githubv4.DeleteDiscussionCommentInput{ - ID: githubv4.ID("DC_kwDOInvalid"), }, - nil, - githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOInvalid'."), - ), - ), - expectToolError: true, - expectedErrMsg: "Could not resolve to a node with the global id of 'DC_kwDOInvalid'.", - }, - { - name: "insufficient permissions", - requestArgs: map[string]any{ - "commentNodeID": "DC_kwDOComment456", - }, - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewMutationMatcher( - struct { - DeleteDiscussionComment struct { - Comment struct { - ID githubv4.ID - URL githubv4.String `graphql:"url"` - } - } `graphql:"deleteDiscussionComment(input: $input)"` - }{}, - githubv4.DeleteDiscussionCommentInput{ - ID: githubv4.ID("DC_kwDOComment456"), + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", }, - nil, - githubv4mock.ErrorResponse("insufficient permissions to delete this discussion comment"), - ), - ), - expectToolError: true, - expectedErrMsg: "insufficient permissions to delete this discussion comment", + "totalCount": 2, + }, + }, }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(tc.mockedClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) + }) - req := createMCPRequest(tc.requestArgs) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) + matcher := githubv4mock.NewQueryMatcher(qWithReplies, vars, mockResponse) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) - text := getTextResult(t, res).Text + reqParams := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "includeReplies": true, + } + req := createMCPRequest(reqParams) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) - if tc.expectToolError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.expectedErrMsg) - return - } + text := getTextResult(t, res).Text + require.False(t, res.IsError, "expected no error, got: %s", text) - require.False(t, res.IsError) - var response MinimalResponse - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Equal(t, tc.expectedID, response.ID) - assert.Equal(t, tc.expectedURL, response.URL) - }) + var response struct { + Comments []MinimalDiscussionComment `json:"comments"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + } `json:"pageInfo"` + TotalCount int `json:"totalCount"` } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Len(t, response.Comments, 2) + + assert.Equal(t, "DC_id1", response.Comments[0].ID) + assert.Equal(t, "Top-level comment", response.Comments[0].Body) + require.Len(t, response.Comments[0].Replies, 1) + assert.Equal(t, "DC_reply1", response.Comments[0].Replies[0].ID) + assert.Equal(t, "Reply to first comment", response.Comments[0].Replies[0].Body) + assert.True(t, response.Comments[0].Replies[0].IsAnswer) + assert.Equal(t, 1, response.Comments[0].ReplyTotalCount) + + assert.Equal(t, "DC_id2", response.Comments[1].ID) + assert.Empty(t, response.Comments[1].Replies) + assert.Equal(t, 0, response.Comments[1].ReplyTotalCount) } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 37e0e6a6ca..af47760065 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -258,10 +258,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { ListDiscussions(t), GetDiscussion(t), GetDiscussionComments(t), - AddDiscussionComment(t), - UpdateDiscussionComment(t), - DeleteDiscussionComment(t), - SetDiscussionCommentAnswer(t), + DiscussionCommentWrite(t), ListDiscussionCategories(t), // Actions tools From b4aedb5fb1b39c824367cca1a6c71809d5a37194 Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Fri, 8 May 2026 12:02:55 +0100 Subject: [PATCH 5/6] add tests cases for checking param presence --- pkg/github/discussions_test.go | 191 +++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 765c88c96f..106bf971ab 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -985,6 +985,22 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectedDiscussionID string expectedDiscussionURL string }{ + { + name: "method: missing", + requestArgs: map[string]any{}, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: method", + }, + { + name: "method: empty", + requestArgs: map[string]any{ + "method": "", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: method", + }, // add method tests { name: "add: successful comment creation", @@ -1084,6 +1100,54 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "insufficient permissions to comment on this discussion", }, + { + name: "add: missing owner", + requestArgs: map[string]any{ + "method": "add", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: owner", + }, + { + name: "add: missing repo", + requestArgs: map[string]any{ + "method": "add", + "owner": "owner", + "discussionNumber": int32(1), + "body": "This is a comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: repo", + }, + { + name: "add: missing discussion number", + requestArgs: map[string]any{ + "method": "add", + "owner": "owner", + "repo": "repo", + "body": "This is a comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: discussionNumber", + }, + { + name: "add: missing body", + requestArgs: map[string]any{ + "method": "add", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: body", + }, // reply method tests { name: "reply: successful reply to comment", @@ -1199,6 +1263,86 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: `commentNodeID "DC_kwDOInvalid" does not resolve to a valid discussion comment`, }, + { + name: "reply: validation query failure", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Node struct { + DiscussionComment struct { + ID githubv4.ID + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + }{}, + map[string]any{ + "replyToID": githubv4.ID("DC_kwDOComment456"), + }, + githubv4mock.ErrorResponse("Could not resolve to a node with the global id of 'DC_kwDOComment456'."), + ), + ), + expectToolError: true, + expectedErrMsg: "failed to validate commentNodeID: Could not resolve to a node with the global id of 'DC_kwDOComment456'.", + }, + { + name: "reply: missing owner", + requestArgs: map[string]any{ + "method": "reply", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: owner", + }, + { + name: "reply: missing repo", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: repo", + }, + { + name: "reply: missing discussion number", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: discussionNumber", + }, + { + name: "reply: missing body", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: body", + }, // update method tests { name: "update: successful comment update", @@ -1291,6 +1435,26 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "insufficient permissions to update this discussion comment", }, + { + name: "update: missing commentNodeID", + requestArgs: map[string]any{ + "method": "update", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: commentNodeID", + }, + { + name: "update: missing body", + requestArgs: map[string]any{ + "method": "update", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: body", + }, // delete method tests { name: "delete: successful comment delete", @@ -1377,6 +1541,15 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "insufficient permissions to delete this discussion comment", }, + { + name: "delete: missing commentNodeID", + requestArgs: map[string]any{ + "method": "delete", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: commentNodeID", + }, // mark_answer method tests { name: "mark_answer: successful mark as answer", @@ -1437,6 +1610,15 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "discussion is not a Q&A discussion", }, + { + name: "mark_answer: missing commentNodeID", + requestArgs: map[string]any{ + "method": "mark_answer", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: commentNodeID", + }, // unmark_answer method tests { name: "unmark_answer: successful unmark as answer", @@ -1497,6 +1679,15 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "insufficient permissions", }, + { + name: "unmark_answer: missing commentNodeID", + requestArgs: map[string]any{ + "method": "unmark_answer", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "missing required parameter: commentNodeID", + }, // invalid method test { name: "invalid method", From 6b6bc178e0ea5d0f818f27f9fc13859d960e62bf Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Fri, 8 May 2026 13:33:56 +0100 Subject: [PATCH 6/6] Enhance validation on discussion comment operations --- pkg/github/discussions.go | 36 ++++++++--- pkg/github/discussions_test.go | 108 ++++++++++++++++++++++++++------- 2 files changed, 113 insertions(+), 31 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 4037a567d7..1b6e21319d 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -732,13 +732,21 @@ func addDiscussionComment(ctx context.Context, client *githubv4.Client, args map return utils.NewToolResultText(string(out)), nil, nil } -func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { +func requiredCommentNodeID(args map[string]any) (string, error) { commentNodeID, err := RequiredParam[string](args, "commentNodeID") if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + return "", err } if strings.TrimSpace(commentNodeID) == "" { - return utils.NewToolResultError("commentNodeID cannot be blank"), nil, nil + return "", fmt.Errorf("commentNodeID cannot be blank") + } + return commentNodeID, nil +} + +func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { + commentNodeID, err := requiredCommentNodeID(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } owner, err := RequiredParam[string](args, "owner") @@ -764,7 +772,10 @@ func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args var nodeQuery struct { Node struct { DiscussionComment struct { - ID githubv4.ID + ID *githubv4.ID + Discussion struct { + ID githubv4.ID + } `graphql:"discussion"` } `graphql:"... on DiscussionComment"` } `graphql:"node(id: $replyToID)"` } @@ -773,7 +784,7 @@ func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args }); err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to validate commentNodeID: %v", err)), nil, nil } - if nodeQuery.Node.DiscussionComment.ID == nil || nodeQuery.Node.DiscussionComment.ID == "" { + if nodeQuery.Node.DiscussionComment.ID == nil || *nodeQuery.Node.DiscussionComment.ID == "" { return utils.NewToolResultError(fmt.Sprintf("commentNodeID %q does not resolve to a valid discussion comment", commentNodeID)), nil, nil } @@ -793,10 +804,15 @@ func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args if err := client.Query(ctx, &q, vars); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if nodeQuery.Node.DiscussionComment.Discussion.ID != q.Repository.Discussion.ID { + return utils.NewToolResultError( + fmt.Sprintf("commentNodeID %q does not belong to discussion #%d in %s/%s", commentNodeID, discussionNumber, owner, repo), + ), nil, nil + } replyToID := githubv4.ID(commentNodeID) input := githubv4.AddDiscussionCommentInput{ - DiscussionID: q.Repository.Discussion.ID, + DiscussionID: nodeQuery.Node.DiscussionComment.Discussion.ID, Body: githubv4.String(body), ReplyToID: &replyToID, } @@ -827,7 +843,7 @@ func replyToDiscussionComment(ctx context.Context, client *githubv4.Client, args } func updateDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") + commentNodeID, err := requiredCommentNodeID(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -867,7 +883,7 @@ func updateDiscussionComment(ctx context.Context, client *githubv4.Client, args } func deleteDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") + commentNodeID, err := requiredCommentNodeID(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -902,7 +918,7 @@ func deleteDiscussionComment(ctx context.Context, client *githubv4.Client, args } func markDiscussionCommentAsAnswer(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") + commentNodeID, err := requiredCommentNodeID(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -937,7 +953,7 @@ func markDiscussionCommentAsAnswer(ctx context.Context, client *githubv4.Client, } func unmarkDiscussionCommentAsAnswer(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { - commentNodeID, err := RequiredParam[string](args, "commentNodeID") + commentNodeID, err := requiredCommentNodeID(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 106bf971ab..25ce7a0df6 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -973,6 +973,16 @@ func Test_DiscussionCommentWrite(t *testing.T) { }, }), ) + replyValidationQuery := struct { + Node struct { + DiscussionComment struct { + ID *githubv4.ID + Discussion struct { + ID githubv4.ID + } `graphql:"discussion"` + } `graphql:"... on DiscussionComment"` + } `graphql:"node(id: $replyToID)"` + }{} tests := []struct { name string @@ -1161,19 +1171,16 @@ func Test_DiscussionCommentWrite(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Node struct { - DiscussionComment struct { - ID githubv4.ID - } `graphql:"... on DiscussionComment"` - } `graphql:"node(id: $replyToID)"` - }{}, + replyValidationQuery, map[string]any{ "replyToID": githubv4.ID("DC_kwDOComment456"), }, githubv4mock.DataResponse(map[string]any{ "node": map[string]any{ "id": "DC_kwDOComment456", + "discussion": map[string]any{ + "id": "D_kwDOTest123", + }, }, }), ), @@ -1245,13 +1252,7 @@ func Test_DiscussionCommentWrite(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Node struct { - DiscussionComment struct { - ID githubv4.ID - } `graphql:"... on DiscussionComment"` - } `graphql:"node(id: $replyToID)"` - }{}, + replyValidationQuery, map[string]any{ "replyToID": githubv4.ID("DC_kwDOInvalid"), }, @@ -1263,6 +1264,36 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: `commentNodeID "DC_kwDOInvalid" does not resolve to a valid discussion comment`, }, + { + name: "reply: comment from another discussion is rejected", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + replyValidationQuery, + map[string]any{ + "replyToID": githubv4.ID("DC_kwDOComment456"), + }, + githubv4mock.DataResponse(map[string]any{ + "node": map[string]any{ + "id": "DC_kwDOComment456", + "discussion": map[string]any{ + "id": "D_kwDOOtherDiscussion456", + }, + }, + }), + ), + discussionQueryMatcher, + ), + expectToolError: true, + expectedErrMsg: `commentNodeID "DC_kwDOComment456" does not belong to discussion #1 in owner/repo`, + }, { name: "reply: validation query failure", requestArgs: map[string]any{ @@ -1275,13 +1306,7 @@ func Test_DiscussionCommentWrite(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Node struct { - DiscussionComment struct { - ID githubv4.ID - } `graphql:"... on DiscussionComment"` - } `graphql:"node(id: $replyToID)"` - }{}, + replyValidationQuery, map[string]any{ "replyToID": githubv4.ID("DC_kwDOComment456"), }, @@ -1445,6 +1470,17 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "missing required parameter: commentNodeID", }, + { + name: "update: whitespace-only commentNodeID is rejected", + requestArgs: map[string]any{ + "method": "update", + "commentNodeID": " ", + "body": "Updated comment text", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "commentNodeID cannot be blank", + }, { name: "update: missing body", requestArgs: map[string]any{ @@ -1550,6 +1586,16 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "missing required parameter: commentNodeID", }, + { + name: "delete: whitespace-only commentNodeID is rejected", + requestArgs: map[string]any{ + "method": "delete", + "commentNodeID": " ", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "commentNodeID cannot be blank", + }, // mark_answer method tests { name: "mark_answer: successful mark as answer", @@ -1619,6 +1665,16 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "missing required parameter: commentNodeID", }, + { + name: "mark_answer: whitespace-only commentNodeID is rejected", + requestArgs: map[string]any{ + "method": "mark_answer", + "commentNodeID": " ", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "commentNodeID cannot be blank", + }, // unmark_answer method tests { name: "unmark_answer: successful unmark as answer", @@ -1688,6 +1744,16 @@ func Test_DiscussionCommentWrite(t *testing.T) { expectToolError: true, expectedErrMsg: "missing required parameter: commentNodeID", }, + { + name: "unmark_answer: whitespace-only commentNodeID is rejected", + requestArgs: map[string]any{ + "method": "unmark_answer", + "commentNodeID": " ", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedErrMsg: "commentNodeID cannot be blank", + }, // invalid method test { name: "invalid method",