Fix granular review findings

Co-authored-by: timrogers <116134+timrogers@users.noreply.github.com>

Author: Copilot

pkg/github/__toolsnaps__/add_issue_comment_ff_issues_granular.snap

@@ -0,0 +1,55 @@
+{
+  "annotations": {
+    "idempotentHint": false,
+    "readOnlyHint": false,
+    "title": "Add comment to issue"
+  },
+  "description": "Add a comment and/or reaction to a specific issue or issue comment in a GitHub repository. Only works on issues; for pull requests use add_pull_request_comment. At least one of body or reaction is required.",
+  "inputSchema": {
+    "properties": {
+      "body": {
+        "description": "Comment content. Required unless reaction is provided.",
+        "minLength": 1,
+        "type": "string"
+      },
+      "comment_id": {
+        "description": "The numeric ID of the issue comment to react to. Use this for reactions to comments; omit it to react to the issue itself. Cannot be combined with body.",
+        "minimum": 1,
+        "type": "integer"
+      },
+      "issue_number": {
+        "description": "Issue number to comment on or react to.",
+        "type": "number"
+      },
+      "owner": {
+        "description": "Repository owner",
+        "type": "string"
+      },
+      "reaction": {
+        "description": "Emoji reaction to add. Required unless body is provided.",
+        "enum": [
+          "+1",
+          "-1",
+          "laugh",
+          "confused",
+          "heart",
+          "hooray",
+          "rocket",
+          "eyes"
+        ],
+        "type": "string"
+      },
+      "repo": {
+        "description": "Repository name",
+        "type": "string"
+      }
+    },
+    "required": [
+      "owner",
+      "repo",
+      "issue_number"
+    ],
+    "type": "object"
+  },
+  "name": "add_issue_comment"
+}

pkg/github/__toolsnaps__/update_issue_comment_ff_issues_granular.snap

@@ -0,0 +1,38 @@
+{
+  "annotations": {
+    "idempotentHint": false,
+    "readOnlyHint": false,
+    "title": "Update issue comment"
+  },
+  "description": "Update the body of an existing issue comment. Only works on issue comments; for pull request conversation comments use update_pull_request_comment.",
+  "inputSchema": {
+    "properties": {
+      "body": {
+        "description": "New comment content",
+        "minLength": 1,
+        "type": "string"
+      },
+      "comment_id": {
+        "description": "The numeric ID of the issue comment to update.",
+        "minimum": 1,
+        "type": "integer"
+      },
+      "owner": {
+        "description": "Repository owner",
+        "type": "string"
+      },
+      "repo": {
+        "description": "Repository name",
+        "type": "string"
+      }
+    },
+    "required": [
+      "owner",
+      "repo",
+      "comment_id",
+      "body"
+    ],
+    "type": "object"
+  },
+  "name": "update_issue_comment"
+}

pkg/github/granular_tools_test.go

@@ -88,6 +88,16 @@ func TestGranularToolSnaps(t *testing.T) {
 			require.NoError(t, toolsnaps.Test(serverTool.Tool.Name, serverTool.Tool))
 		})
 	}
+
+	for _, constructor := range []func(translations.TranslationHelperFunc) inventory.ServerTool{
+		GranularAddIssueComment,
+		GranularUpdateIssueComment,
+	} {
+		serverTool := constructor(translations.NullTranslationHelper)
+		t.Run(serverTool.Tool.Name+"_ff_"+FeatureFlagIssuesGranular, func(t *testing.T) {
+			require.NoError(t, toolsnaps.Test(serverTool.Tool.Name+"_ff_"+FeatureFlagIssuesGranular, serverTool.Tool))
+		})
+	}
 }
 
 func TestIssuesGranularToolset(t *testing.T) {

pkg/github/issues.go

@@ -1491,7 +1491,7 @@ func addIssueComment(t translations.TranslationHelperFunc, issueOnly bool) inven
 			if issueOnly {
 				// A comment_id is validated against issue_number below, so checking
 				// issue_number also covers reactions to comments.
-				if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+				if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "add_issue_comment"); result != nil {
 					return result, nil, nil
 				}
 			}
@@ -1676,7 +1676,7 @@ func updateIssueComment(t translations.TranslationHelperFunc, issueOnly bool) in
 			}
 
 			if issueOnly {
-				if result := EnsureIssueComment(ctx, client, owner, repo, commentID); result != nil {
+				if result := EnsureIssueComment(ctx, client, owner, repo, commentID, "update_issue_comment"); result != nil {
 					return result, nil, nil
 				}
 			}

pkg/github/issues_granular.go

@@ -27,33 +27,43 @@ func normalizeConfidence(confidence string) string {
 // rather than an issue. GitHub's issues API accepts pull request numbers, so tools
 // that should only act on issues must check this explicitly. A nil result means
 // the number refers to an issue.
-func EnsureIssue(ctx context.Context, client *github.Client, owner, repo string, issueNumber int) *mcp.CallToolResult {
-	return ensureIssueKind(ctx, client, owner, repo, issueNumber, false)
+func EnsureIssue(ctx context.Context, client *github.Client, owner, repo string, issueNumber int, companionTool ...string) *mcp.CallToolResult {
+	return ensureIssueKind(ctx, client, owner, repo, issueNumber, false, firstOrDefault(companionTool, "the pull request tools"))
 }
 
 // EnsurePullRequest returns an error result if the given number refers to an issue
 // rather than a pull request. A nil result means the number refers to a pull request.
-func EnsurePullRequest(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) *mcp.CallToolResult {
-	return ensureIssueKind(ctx, client, owner, repo, pullNumber, true)
+func EnsurePullRequest(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, companionTool ...string) *mcp.CallToolResult {
+	return ensureIssueKind(ctx, client, owner, repo, pullNumber, true, firstOrDefault(companionTool, "the issue tools"))
 }
 
 // EnsureIssueComment returns an error result if the given conversation comment
 // belongs to a pull request rather than an issue. A nil result means the comment
 // belongs to an issue.
-func EnsureIssueComment(ctx context.Context, client *github.Client, owner, repo string, commentID int64) *mcp.CallToolResult {
-	return ensureCommentKind(ctx, client, owner, repo, commentID, false)
+func EnsureIssueComment(ctx context.Context, client *github.Client, owner, repo string, commentID int64, companionTool ...string) *mcp.CallToolResult {
+	return ensureCommentKind(ctx, client, owner, repo, commentID, false, firstOrDefault(companionTool, "the pull request comment tools"))
 }
 
 // EnsurePullRequestComment returns an error result if the given conversation
 // comment belongs to an issue rather than a pull request. A nil result means the
 // comment belongs to a pull request.
-func EnsurePullRequestComment(ctx context.Context, client *github.Client, owner, repo string, commentID int64) *mcp.CallToolResult {
-	return ensureCommentKind(ctx, client, owner, repo, commentID, true)
+func EnsurePullRequestComment(ctx context.Context, client *github.Client, owner, repo string, commentID int64, companionTool ...string) *mcp.CallToolResult {
+	return ensureCommentKind(ctx, client, owner, repo, commentID, true, firstOrDefault(companionTool, "the issue comment tools"))
 }
 
-func ensureCommentKind(ctx context.Context, client *github.Client, owner, repo string, commentID int64, wantPullRequest bool) *mcp.CallToolResult {
+func firstOrDefault(values []string, fallback string) string {
+	if len(values) > 0 {
+		return values[0]
+	}
+	return fallback
+}
+
+func ensureCommentKind(ctx context.Context, client *github.Client, owner, repo string, commentID int64, wantPullRequest bool, companionTool string) *mcp.CallToolResult {
 	comment, resp, err := client.Issues.GetComment(ctx, owner, repo, commentID)
 	if err != nil {
+		if resp != nil && resp.Body != nil {
+			_ = resp.Body.Close()
+		}
 		return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to get comment %d", commentID), resp, err)
 	}
 	_ = resp.Body.Close()
@@ -62,22 +72,25 @@ func ensureCommentKind(ctx context.Context, client *github.Client, owner, repo s
 	if err != nil {
 		return utils.NewToolResultErrorFromErr(fmt.Sprintf("failed to determine the issue or pull request for comment %d", commentID), err)
 	}
-	return ensureIssueKind(ctx, client, owner, repo, number, wantPullRequest)
+	return ensureIssueKind(ctx, client, owner, repo, number, wantPullRequest, companionTool)
 }
 
-func ensureIssueKind(ctx context.Context, client *github.Client, owner, repo string, number int, wantPullRequest bool) *mcp.CallToolResult {
+func ensureIssueKind(ctx context.Context, client *github.Client, owner, repo string, number int, wantPullRequest bool, companionTool string) *mcp.CallToolResult {
 	issue, resp, err := client.Issues.Get(ctx, owner, repo, number)
 	if err != nil {
+		if resp != nil && resp.Body != nil {
+			_ = resp.Body.Close()
+		}
 		return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to get #%d", number), resp, err)
 	}
 	_ = resp.Body.Close()
 
 	isPullRequest := issue.IsPullRequest()
 	switch {
 	case isPullRequest && !wantPullRequest:
-		return utils.NewToolResultError(fmt.Sprintf("#%d in %s/%s is a pull request, not an issue. Use the pull request tools to modify it.", number, owner, repo))
+		return utils.NewToolResultError(fmt.Sprintf("#%d in %s/%s is a pull request, not an issue. Use %s to modify it.", number, owner, repo, companionTool))
 	case !isPullRequest && wantPullRequest:
-		return utils.NewToolResultError(fmt.Sprintf("#%d in %s/%s is an issue, not a pull request. Use the issue tools to modify it.", number, owner, repo))
+		return utils.NewToolResultError(fmt.Sprintf("#%d in %s/%s is an issue, not a pull request. Use %s to modify it.", number, owner, repo, companionTool))
 	}
 	return nil
 }
@@ -151,7 +164,7 @@ func issueUpdateTool(
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, strings.Replace(name, "update_issue_", "update_pull_request_", 1)); result != nil {
 				return result, nil, nil
 			}
 
@@ -488,7 +501,7 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "update_issue_assignees"); result != nil {
 				return result, nil, nil
 			}
 
@@ -710,7 +723,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "update_issue_labels"); result != nil {
 				return result, nil, nil
 			}
 
@@ -897,7 +910,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "update_issue_type"); result != nil {
 				return result, nil, nil
 			}
 
@@ -1087,7 +1100,7 @@ func GranularUpdateIssueState(t translations.TranslationHelperFunc) inventory.Se
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "update_issue_state"); result != nil {
 				return result, nil, nil
 			}
 
@@ -1725,7 +1738,7 @@ func GranularAddIssueReaction(t translations.TranslationHelperFunc) inventory.Se
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "add_issue_reaction"); result != nil {
 				return result, nil, nil
 			}
 
@@ -1811,7 +1824,7 @@ func GranularRemoveIssueReaction(t translations.TranslationHelperFunc) inventory
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssue(ctx, client, owner, repo, issueNumber); result != nil {
+			if result := EnsureIssue(ctx, client, owner, repo, issueNumber, "remove_issue_reaction"); result != nil {
 				return result, nil, nil
 			}
 
@@ -1892,7 +1905,7 @@ func GranularAddIssueCommentReaction(t translations.TranslationHelperFunc) inven
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssueComment(ctx, client, owner, repo, commentID); result != nil {
+			if result := EnsureIssueComment(ctx, client, owner, repo, commentID, "add_issue_comment_reaction"); result != nil {
 				return result, nil, nil
 			}
 
@@ -1978,7 +1991,7 @@ func GranularRemoveIssueCommentReaction(t translations.TranslationHelperFunc) in
 				return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
 			}
 
-			if result := EnsureIssueComment(ctx, client, owner, repo, commentID); result != nil {
+			if result := EnsureIssueComment(ctx, client, owner, repo, commentID, "remove_issue_comment_reaction"); result != nil {
 				return result, nil, nil
 			}