Validate classifier and IDs before calling the GitHub API

JSON schema constraints (enum, minimum) aren't enforced when tool
arguments are unmarshalled, so an invalid classifier or a negative ID
previously reached GitHub and surfaced as a confusing 404 or GraphQL
error. Reject them up front with a clear argument error instead.

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

Author: timrogers

pkg/github/comment_minimize.go

@@ -3,7 +3,9 @@ package github
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"maps"
+	"slices"
 	"strings"
 
 	ghErrors "github.com/github/github-mcp-server/pkg/errors"
@@ -61,7 +63,7 @@ var issueCommentVisibilityTarget = commentVisibilityTarget{
 	},
 	required: []string{"comment_id"},
 	resolveNodeID: func(ctx context.Context, client *github.Client, owner, repo string, args map[string]any) (string, *mcp.CallToolResult) {
-		commentID, err := RequiredBigInt(args, "comment_id")
+		commentID, err := requiredPositiveBigInt(args, "comment_id")
 		if err != nil {
 			return "", utils.NewToolResultError(err.Error())
 		}
@@ -88,7 +90,7 @@ var pullRequestReviewCommentVisibilityTarget = commentVisibilityTarget{
 	},
 	required: []string{"comment_id"},
 	resolveNodeID: func(ctx context.Context, client *github.Client, owner, repo string, args map[string]any) (string, *mcp.CallToolResult) {
-		commentID, err := RequiredBigInt(args, "comment_id")
+		commentID, err := requiredPositiveBigInt(args, "comment_id")
 		if err != nil {
 			return "", utils.NewToolResultError(err.Error())
 		}
@@ -124,7 +126,10 @@ var pullRequestReviewVisibilityTarget = commentVisibilityTarget{
 		if err != nil {
 			return "", utils.NewToolResultError(err.Error())
 		}
-		reviewID, err := RequiredBigInt(args, "review_id")
+		if pullNumber < 1 {
+			return "", utils.NewToolResultError("pullNumber must be greater than 0")
+		}
+		reviewID, err := requiredPositiveBigInt(args, "review_id")
 		if err != nil {
 			return "", utils.NewToolResultError(err.Error())
 		}
@@ -204,6 +209,10 @@ func setCommentVisibility(ctx context.Context, deps ToolDependencies, target com
 		if err != nil {
 			return utils.NewToolResultError(err.Error())
 		}
+		classifier = strings.ToUpper(classifier)
+		if !slices.Contains(commentClassifiers, any(classifier)) {
+			return utils.NewToolResultError(fmt.Sprintf("invalid classifier %q: must be one of %v", classifier, commentClassifiers))
+		}
 	}
 
 	client, err := deps.GetClient(ctx)
@@ -223,7 +232,7 @@ func setCommentVisibility(ctx context.Context, deps ToolDependencies, target com
 
 	var result MinimizeCommentResult
 	if hide {
-		result, errResult = minimizeComment(ctx, gqlClient, nodeID, strings.ToUpper(classifier))
+		result, errResult = minimizeComment(ctx, gqlClient, nodeID, classifier)
 	} else {
 		result, errResult = unminimizeComment(ctx, gqlClient, nodeID)
 	}
@@ -238,6 +247,19 @@ func setCommentVisibility(ctx context.Context, deps ToolDependencies, target com
 	return utils.NewToolResultText(string(r))
 }
 
+// requiredPositiveBigInt reads a required ID argument. The schema's minimum is not enforced
+// when arguments are unmarshalled, so negative values are rejected here before any API call.
+func requiredPositiveBigInt(args map[string]any, p string) (int64, error) {
+	v, err := RequiredBigInt(args, p)
+	if err != nil {
+		return 0, err
+	}
+	if v < 1 {
+		return 0, fmt.Errorf("%s must be greater than 0", p)
+	}
+	return v, nil
+}
+
 // nodeIDFromResponse turns the result of a REST lookup into a GraphQL node ID, since the
 // minimize mutations only accept node IDs.
 func nodeIDFromResponse(ctx context.Context, errMessage, nodeID string, resp *github.Response, err error) (string, *mcp.CallToolResult) {

pkg/github/comment_minimize_test.go

@@ -282,6 +282,36 @@ func Test_HideAndUnhideComments(t *testing.T) {
 			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "comment_id": float64(1)},
 			expectedErrMsg: "failed to unminimize comment",
 		},
+		{
+			name:           "invalid classifier is rejected before any API call",
+			tool:           GranularHideIssueComment(translations.NullTranslationHelper),
+			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "comment_id": float64(1), "classifier": "BOGUS"},
+			expectedErrMsg: `invalid classifier "BOGUS"`,
+		},
+		{
+			name:           "negative issue comment_id",
+			tool:           GranularHideIssueComment(translations.NullTranslationHelper),
+			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "comment_id": float64(-1), "classifier": "SPAM"},
+			expectedErrMsg: "comment_id must be greater than 0",
+		},
+		{
+			name:           "negative pull request review comment_id",
+			tool:           GranularUnhidePullRequestReviewComment(translations.NullTranslationHelper),
+			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "comment_id": float64(-2)},
+			expectedErrMsg: "comment_id must be greater than 0",
+		},
+		{
+			name:           "negative review_id",
+			tool:           GranularUnhidePullRequestReview(translations.NullTranslationHelper),
+			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "pullNumber": float64(42), "review_id": float64(-3)},
+			expectedErrMsg: "review_id must be greater than 0",
+		},
+		{
+			name:           "negative pullNumber",
+			tool:           GranularUnhidePullRequestReview(translations.NullTranslationHelper),
+			requestArgs:    map[string]any{"owner": "owner", "repo": "repo", "pullNumber": float64(-42), "review_id": float64(3)},
+			expectedErrMsg: "pullNumber must be greater than 0",
+		},
 		{
 			name:           "missing owner",
 			tool:           GranularUnhideIssueComment(translations.NullTranslationHelper),