Skip to content

Commit e522c77

Browse files
SamMorrowDrumsCopilot
SamMorrowDrums
and
Copilot
committed
Validate issue comment reaction target
Use the required issue_number to verify issue comment reaction targets before creating the reaction, and remove overlapping add_issue_comment test coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ca55793 commit e522c77

3 files changed

Lines changed: 65 additions & 106 deletions

File tree

‎pkg/github/helper_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ const (
5858

5959
// Issues endpoints
6060
GetReposIssuesByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}"
61+
GetReposIssuesCommentByOwnerByRepoByCommentID = "GET /repos/{owner}/{repo}/issues/comments/{comment_id}"
6162
GetReposIssuesCommentsByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}/comments"
6263
PostReposIssuesByOwnerByRepo = "POST /repos/{owner}/{repo}/issues"
6364
PostReposIssuesCommentsByOwnerByRepoByIssueNumber = "POST /repos/{owner}/{repo}/issues/{issue_number}/comments"

‎pkg/github/issues.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,6 +1200,20 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
12001200
var reactionResponse *MinimalResponse
12011201
if hasReaction {
12021202
if hasCommentID {
1203+
comment, resp, err := client.Issues.GetComment(ctx, owner, repo, commentID)
1204+
if err != nil {
1205+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get issue comment", resp, err), nil, nil
1206+
}
1207+
defer func() { _ = resp.Body.Close() }()
1208+
1209+
commentIssueNumber, err := issueNumberFromIssueURL(comment.GetIssueURL())
1210+
if err != nil {
1211+
return utils.NewToolResultErrorFromErr("failed to determine issue number for comment", err), nil, nil
1212+
}
1213+
if commentIssueNumber != issueNumber {
1214+
return utils.NewToolResultError(fmt.Sprintf("comment_id does not belong to issue_number %d", issueNumber)), nil, nil
1215+
}
1216+
12031217
reaction, resp, err := client.Reactions.CreateIssueCommentReaction(ctx, owner, repo, commentID, reactionContent)
12041218
if err != nil {
12051219
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to add reaction to issue comment", resp, err), nil, nil
@@ -1271,6 +1285,15 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
12711285
})
12721286
}
12731287

1288+
func issueNumberFromIssueURL(issueURL string) (int, error) {
1289+
issueNumberString := issueURL[strings.LastIndex(issueURL, "/")+1:]
1290+
issueNumber, err := strconv.Atoi(issueNumberString)
1291+
if err != nil {
1292+
return 0, fmt.Errorf("invalid issue URL %q: %w", issueURL, err)
1293+
}
1294+
return issueNumber, nil
1295+
}
1296+
12741297
// SubIssueWrite creates a tool to add a sub-issue to a parent issue.
12751298
func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool {
12761299
st := NewTool(

‎pkg/github/issues_test.go‎

Lines changed: 41 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -560,112 +560,6 @@ func Test_GetIssue_FieldValues_Enriched(t *testing.T) {
560560
assert.Equal(t, "2.5", returnedIssue.FieldValues[1].Value)
561561
}
562562

563-
func Test_AddIssueComment(t *testing.T) {
564-
// Verify tool definition once
565-
serverTool := AddIssueComment(translations.NullTranslationHelper)
566-
tool := serverTool.Tool
567-
require.NoError(t, toolsnaps.Test(tool.Name, tool))
568-
569-
assert.Equal(t, "add_issue_comment", tool.Name)
570-
assert.NotEmpty(t, tool.Description)
571-
572-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "owner")
573-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "repo")
574-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "issue_number")
575-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "comment_id")
576-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "body")
577-
assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "reaction")
578-
assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"owner", "repo", "issue_number"})
579-
580-
// Setup mock comment for success case
581-
mockComment := &github.IssueComment{
582-
ID: github.Ptr(int64(123)),
583-
Body: github.Ptr("This is a test comment"),
584-
User: &github.User{
585-
Login: github.Ptr("testuser"),
586-
},
587-
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42#issuecomment-123"),
588-
}
589-
590-
tests := []struct {
591-
name string
592-
mockedClient *http.Client
593-
requestArgs map[string]any
594-
expectError bool
595-
expectedComment *github.IssueComment
596-
expectedErrMsg string
597-
}{
598-
{
599-
name: "successful comment creation",
600-
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
601-
PostReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusCreated, mockComment),
602-
}),
603-
requestArgs: map[string]any{
604-
"owner": "owner",
605-
"repo": "repo",
606-
"issue_number": float64(42),
607-
"body": "This is a test comment",
608-
},
609-
expectError: false,
610-
expectedComment: mockComment,
611-
},
612-
{
613-
name: "comment creation fails",
614-
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
615-
PostReposIssuesCommentsByOwnerByRepoByIssueNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
616-
w.WriteHeader(http.StatusUnprocessableEntity)
617-
_, _ = w.Write([]byte(`{"message": "Invalid request"}`))
618-
}),
619-
}),
620-
requestArgs: map[string]any{
621-
"owner": "owner",
622-
"repo": "repo",
623-
"issue_number": float64(42),
624-
"body": "This is a test comment",
625-
},
626-
expectError: true,
627-
expectedErrMsg: "failed to create comment",
628-
},
629-
}
630-
631-
for _, tc := range tests {
632-
t.Run(tc.name, func(t *testing.T) {
633-
// Setup client with mock
634-
client := mustNewGHClient(t, tc.mockedClient)
635-
deps := BaseDeps{
636-
Client: client,
637-
}
638-
handler := serverTool.Handler(deps)
639-
640-
// Create call request
641-
request := createMCPRequest(tc.requestArgs)
642-
643-
// Call handler
644-
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
645-
646-
if tc.expectError {
647-
require.NoError(t, err)
648-
require.True(t, result.IsError)
649-
errorContent := getErrorResult(t, result)
650-
assert.Contains(t, errorContent.Text, tc.expectedErrMsg)
651-
return
652-
}
653-
654-
require.NoError(t, err)
655-
656-
// Parse the result and get the text content if no error
657-
textContent := getTextResult(t, result)
658-
659-
// Unmarshal and verify the result contains minimal response
660-
var minimalResponse MinimalResponse
661-
err = json.Unmarshal([]byte(textContent.Text), &minimalResponse)
662-
require.NoError(t, err)
663-
assert.Equal(t, fmt.Sprintf("%d", tc.expectedComment.GetID()), minimalResponse.ID)
664-
assert.Equal(t, tc.expectedComment.GetHTMLURL(), minimalResponse.URL)
665-
})
666-
}
667-
}
668-
669563
func Test_SearchIssues(t *testing.T) {
670564
// Verify tool definition once
671565
serverTool := SearchIssues(translations.NullTranslationHelper)
@@ -4316,6 +4210,10 @@ func TestAddIssueComment(t *testing.T) {
43164210
ID: github.Ptr(int64(789)),
43174211
Content: github.Ptr("heart"),
43184212
}
4213+
mockIssueComment := &github.IssueComment{
4214+
ID: github.Ptr(int64(999)),
4215+
IssueURL: github.Ptr("https://api.github.com/repos/owner/repo/issues/42"),
4216+
}
43194217
commentCreatedAfterReactionFailure := &atomic.Bool{}
43204218

43214219
tests := []struct {
@@ -4353,6 +4251,7 @@ func TestAddIssueComment(t *testing.T) {
43534251
{
43544252
name: "successful reaction to issue comment",
43554253
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
4254+
GetReposIssuesCommentByOwnerByRepoByCommentID: mockResponse(t, http.StatusOK, mockIssueComment),
43564255
PostReposIssuesCommentsReactionsByOwnerByRepoByCommentID: mockResponse(t, http.StatusCreated, mockReaction),
43574256
}),
43584257
requestArgs: map[string]any{
@@ -4363,6 +4262,42 @@ func TestAddIssueComment(t *testing.T) {
43634262
"reaction": "heart",
43644263
},
43654264
},
4265+
{
4266+
name: "issue comment reaction requires matching issue_number",
4267+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
4268+
GetReposIssuesCommentByOwnerByRepoByCommentID: mockResponse(t, http.StatusOK, &github.IssueComment{
4269+
ID: github.Ptr(int64(999)),
4270+
IssueURL: github.Ptr("https://api.github.com/repos/owner/repo/issues/43"),
4271+
}),
4272+
}),
4273+
requestArgs: map[string]any{
4274+
"owner": "owner",
4275+
"repo": "repo",
4276+
"issue_number": float64(42),
4277+
"comment_id": float64(999),
4278+
"reaction": "heart",
4279+
},
4280+
expectToolError: true,
4281+
expectedToolErrMsg: "comment_id does not belong to issue_number 42",
4282+
},
4283+
{
4284+
name: "issue comment lookup fails",
4285+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
4286+
GetReposIssuesCommentByOwnerByRepoByCommentID: func(w http.ResponseWriter, _ *http.Request) {
4287+
w.WriteHeader(http.StatusNotFound)
4288+
_, _ = w.Write([]byte(`{"message": "Not Found"}`))
4289+
},
4290+
}),
4291+
requestArgs: map[string]any{
4292+
"owner": "owner",
4293+
"repo": "repo",
4294+
"issue_number": float64(42),
4295+
"comment_id": float64(999),
4296+
"reaction": "heart",
4297+
},
4298+
expectToolError: true,
4299+
expectedToolErrMsg: "failed to get issue comment",
4300+
},
43664301
{
43674302
name: "successful comment and reaction to issue",
43684303
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{

0 commit comments

Comments
 (0)