From 2ef13f110d370c28300cd519766144ea35f630ac Mon Sep 17 00:00:00 2001 From: Ksenia Bobrova Date: Tue, 3 Mar 2026 15:49:33 +0100 Subject: [PATCH 1/3] Gracefully handle numeric parameters passed as strings --- pkg/github/copilot.go | 2 +- pkg/github/copilot_test.go | 109 ++++++++++++++++++++++++++++++ pkg/github/discussions.go | 4 +- pkg/github/discussions_test.go | 105 +++++++++++++++++++++++++++++ pkg/github/params.go | 76 ++++++++++++++++----- pkg/github/params_test.go | 63 ++++++++++++++++++ pkg/github/pullrequests.go | 4 +- pkg/github/pullrequests_test.go | 114 ++++++++++++++++++++++++++++++++ 8 files changed, 455 insertions(+), 22 deletions(-) diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 525a58c69..d95357e73 100644 --- a/pkg/github/copilot.go +++ b/pkg/github/copilot.go @@ -209,7 +209,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server BaseRef string `mapstructure:"base_ref"` CustomInstructions string `mapstructure:"custom_instructions"` } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/copilot_test.go b/pkg/github/copilot_test.go index 6b1e7c990..0a1d5ef3b 100644 --- a/pkg/github/copilot_test.go +++ b/pkg/github/copilot_test.go @@ -165,6 +165,115 @@ func TestAssignCopilotToIssue(t *testing.T) { ), ), }, + { + name: "successful assignment with string issue_number", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": "123", // Some MCP clients send numeric values as strings + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + SuggestedActors struct { + Nodes []struct { + Bot struct { + ID githubv4.ID + Login githubv4.String + TypeName string `graphql:"__typename"` + } `graphql:"... on Bot"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"suggestedActors(first: 100, after: $endCursor, capabilities: CAN_BE_ASSIGNED)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "endCursor": (*githubv4.String)(nil), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "suggestedActors": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("copilot-swe-agent-id"), + "login": githubv4.String("copilot-swe-agent"), + "__typename": "Bot", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + ID githubv4.ID + Issue struct { + ID githubv4.ID + Assignees struct { + Nodes []struct { + ID githubv4.ID + } + } `graphql:"assignees(first: 100)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "number": githubv4.Int(123), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "id": githubv4.ID("test-repo-id"), + "issue": map[string]any{ + "id": githubv4.ID("test-issue-id"), + "assignees": map[string]any{ + "nodes": []any{}, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + UpdateIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + } + } `graphql:"updateIssue(input: $input)"` + }{}, + UpdateIssueInput{ + ID: githubv4.ID("test-issue-id"), + AssigneeIDs: []githubv4.ID{githubv4.ID("copilot-swe-agent-id")}, + AgentAssignment: &AgentAssignmentInput{ + BaseRef: nil, + CustomAgent: ptrGitHubv4String(""), + CustomInstructions: ptrGitHubv4String(""), + TargetRepositoryID: githubv4.ID("test-repo-id"), + }, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateIssue": map[string]any{ + "issue": map[string]any{ + "id": githubv4.ID("test-issue-id"), + "number": githubv4.Int(123), + "url": githubv4.String("https://github.com/owner/repo/issues/123"), + }, + }, + }), + ), + ), + }, { name: "successful assignment when there are existing assignees", requestArgs: map[string]any{ diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 6971bab07..700560b47 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -313,7 +313,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { Repo string DiscussionNumber int32 } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetGQLClient(ctx) @@ -417,7 +417,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve Repo string DiscussionNumber int32 } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 998a6471b..692ef2ec8 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -590,6 +590,50 @@ func Test_GetDiscussion(t *testing.T) { } } +func Test_GetDiscussionWithStringNumber(t *testing.T) { + // Test that WeakDecode handles string discussionNumber from MCP clients + toolDef := GetDiscussion(translations.NullTranslationHelper) + + qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,title,body,createdAt,closed,isAnswered,answerChosenAt,url,category{name}}}}" + + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + } + + matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{"discussion": map[string]any{ + "number": 1, + "title": "Test Discussion Title", + "body": "This is a test discussion", + "url": "https://github.com/owner/repo/discussions/1", + "createdAt": "2025-04-25T12:00:00Z", + "closed": false, + "isAnswered": false, + "category": map[string]any{"name": "General"}, + }}, + })) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + // Send discussionNumber as a string instead of a number + reqParams := map[string]any{"owner": "owner", "repo": "repo", "discussionNumber": "1"} + 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 out map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &out)) + assert.Equal(t, float64(1), out["number"]) + assert.Equal(t, "Test Discussion Title", out["title"]) +} + func Test_GetDiscussionComments(t *testing.T) { // Verify tool definition and schema toolDef := GetDiscussionComments(translations.NullTranslationHelper) @@ -675,6 +719,67 @@ func Test_GetDiscussionComments(t *testing.T) { } } +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}}}}" + + 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{ + {"body": "First comment"}, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 1, + }, + }, + }, + }) + matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + // Send discussionNumber as a string instead of a number + reqParams := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": "1", + } + request := createMCPRequest(reqParams) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + require.False(t, result.IsError, "expected no error, got: %s", textContent.Text) + + var out struct { + Comments []map[string]any `json:"comments"` + TotalCount int `json:"totalCount"` + } + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out)) + assert.Len(t, out.Comments, 1) + assert.Equal(t, "First comment", out.Comments[0]["body"]) +} + func Test_ListDiscussionCategories(t *testing.T) { toolDef := ListDiscussionCategories(translations.NullTranslationHelper) tool := toolDef.Tool diff --git a/pkg/github/params.go b/pkg/github/params.go index 0dac1773f..b839544a8 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -39,6 +39,23 @@ func isAcceptedError(err error) bool { return errors.As(err, &acceptedError) } +// toFloat64 attempts to convert a value to float64, handling both numeric and +// string representations. Some MCP clients send numeric values as strings. +func toFloat64(val any) (float64, bool) { + switch v := val.(type) { + case float64: + return v, true + case string: + f, err := strconv.ParseFloat(v, 64) + if err != nil { + return 0, false + } + return f, true + default: + return 0, false + } +} + // RequiredParam is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. @@ -68,32 +85,51 @@ func RequiredParam[T comparable](args map[string]any, p string) (T, error) { // RequiredInt is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. -// 2. Checks if the parameter is of the expected type. +// 2. Checks if the parameter is of the expected type (float64 or numeric string). // 3. Checks if the parameter is not empty, i.e: non-zero value func RequiredInt(args map[string]any, p string) (int, error) { - v, err := RequiredParam[float64](args, p) - if err != nil { - return 0, err + v, ok := args[p] + if !ok { + return 0, fmt.Errorf("missing required parameter: %s", p) + } + + f, ok := toFloat64(v) + if !ok { + return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, v) + } + + if f == 0 { + return 0, fmt.Errorf("missing required parameter: %s", p) } - return int(v), nil + + return int(f), nil } // RequiredBigInt is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. -// 2. Checks if the parameter is of the expected type (float64). +// 2. Checks if the parameter is of the expected type (float64 or numeric string). // 3. Checks if the parameter is not empty, i.e: non-zero value. // 4. Validates that the float64 value can be safely converted to int64 without truncation. func RequiredBigInt(args map[string]any, p string) (int64, error) { - v, err := RequiredParam[float64](args, p) - if err != nil { - return 0, err + val, ok := args[p] + if !ok { + return 0, fmt.Errorf("missing required parameter: %s", p) + } + + f, ok := toFloat64(val) + if !ok { + return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, val) + } + + if f == 0 { + return 0, fmt.Errorf("missing required parameter: %s", p) } - result := int64(v) + result := int64(f) // Check if converting back produces the same value to avoid silent truncation - if float64(result) != v { - return 0, fmt.Errorf("parameter %s value %f is too large to fit in int64", p, v) + if float64(result) != f { + return 0, fmt.Errorf("parameter %s value %f is too large to fit in int64", p, f) } return result, nil } @@ -121,13 +157,19 @@ func OptionalParam[T any](args map[string]any, p string) (T, error) { // OptionalIntParam is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request, if not, it returns its zero-value -// 2. If it is present, it checks if the parameter is of the expected type and returns it +// 2. If it is present, it checks if the parameter is of the expected type (float64 or numeric string) and returns it func OptionalIntParam(args map[string]any, p string) (int, error) { - v, err := OptionalParam[float64](args, p) - if err != nil { - return 0, err + val, ok := args[p] + if !ok { + return 0, nil } - return int(v), nil + + f, ok := toFloat64(val) + if !ok { + return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, val) + } + + return int(f), nil } // OptionalIntParamWithDefault is a helper function that can be used to fetch a requested parameter from the request diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index 5c989d55a..d75850b2e 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -163,6 +163,13 @@ func Test_RequiredInt(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -170,6 +177,13 @@ func Test_RequiredInt(t *testing.T) { expected: 0, expectError: true, }, + { + name: "zero string parameter", + params: map[string]any{"count": "0"}, + paramName: "count", + expected: 0, + expectError: true, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -177,6 +191,13 @@ func Test_RequiredInt(t *testing.T) { expected: 0, expectError: true, }, + { + name: "boolean type parameter", + params: map[string]any{"count": true}, + paramName: "count", + expected: 0, + expectError: true, + }, } for _, tc := range tests { @@ -207,6 +228,13 @@ func Test_OptionalIntParam(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -221,6 +249,13 @@ func Test_OptionalIntParam(t *testing.T) { expected: 0, expectError: false, }, + { + name: "zero string value", + params: map[string]any{"count": "0"}, + paramName: "count", + expected: 0, + expectError: false, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -261,6 +296,14 @@ func Test_OptionalNumberParamWithDefault(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + defaultVal: 10, + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -277,6 +320,14 @@ func Test_OptionalNumberParamWithDefault(t *testing.T) { expected: 10, expectError: false, }, + { + name: "zero string value uses default", + params: map[string]any{"count": "0"}, + paramName: "count", + defaultVal: 10, + expected: 10, + expectError: false, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -486,6 +537,18 @@ func TestOptionalPaginationParams(t *testing.T) { expected: PaginationParams{}, expectError: true, }, + { + name: "string page and perPage parameters", + params: map[string]any{ + "page": "3", + "perPage": "25", + }, + expected: PaginationParams{ + Page: 3, + PerPage: 25, + }, + expectError: false, + }, } for _, tc := range tests { diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 7eb9c6d64..e5e0855ea 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1570,7 +1570,7 @@ Available methods: []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { var params PullRequestReviewWriteParams - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -1905,7 +1905,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S StartLine *int32 StartSide *string } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 4bb31e541..537577329 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2470,6 +2470,61 @@ func TestCreateAndSubmitPullRequestReview(t *testing.T) { }, expectToolError: false, }, + { + name: "successful review creation with string pullNumber", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + }, + }, + }, + ), + ), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReview struct { + PullRequestReview struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReview(input: $input)"` + }{}, + githubv4.AddPullRequestReviewInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + Body: githubv4.NewString("This is a test review"), + Event: githubv4mock.Ptr(githubv4.PullRequestReviewEventComment), + CommitOID: githubv4.NewGitObjectID("abcd1234"), + }, + nil, + githubv4mock.DataResponse(map[string]any{}), + ), + ), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "pullNumber": "42", // Some MCP clients send numeric values as strings + "body": "This is a test review", + "event": "COMMENT", + "commitID": "abcd1234", + }, + expectToolError: false, + }, { name: "failure to get pull request", mockedClient: githubv4mock.NewMockedHTTPClient( @@ -2873,6 +2928,65 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { ), ), }, + { + name: "successful line comment with string pullNumber and line", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": "42", // Some MCP clients send numeric values as strings + "path": "file.go", + "body": "This is a test comment", + "subjectType": "LINE", + "line": "10", // string line number + "side": "RIGHT", + "startLine": "5", // string startLine + "startSide": "RIGHT", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + viewerQuery("williammartin"), + getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ + author: "williammartin", + owner: "owner", + repo: "repo", + prNum: 42, + + reviews: []getLatestPendingReviewQueryReview{ + { + id: "PR_kwDODKw3uc6WYN1T", + state: "PENDING", + url: "https://github.com/owner/repo/pull/42", + }, + }, + }), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReviewThread struct { + Thread struct { + ID githubv4.String + } + } `graphql:"addPullRequestReviewThread(input: $input)"` + }{}, + githubv4.AddPullRequestReviewThreadInput{ + Path: githubv4.String("file.go"), + Body: githubv4.String("This is a test comment"), + SubjectType: githubv4mock.Ptr(githubv4.PullRequestReviewThreadSubjectTypeLine), + Line: githubv4.NewInt(10), + Side: githubv4mock.Ptr(githubv4.DiffSideRight), + StartLine: githubv4.NewInt(5), + StartSide: githubv4mock.Ptr(githubv4.DiffSideRight), + PullRequestReviewID: githubv4.NewID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addPullRequestReviewThread": map[string]any{ + "thread": map[string]any{ + "id": "MDEyOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTIzNDU2", + }, + }, + }), + ), + ), + }, { name: "thread ID is nil - invalid line number", requestArgs: map[string]any{ From f7850456416e80983c027a2b13d7d9f153162a65 Mon Sep 17 00:00:00 2001 From: Ksenia Bobrova Date: Tue, 3 Mar 2026 16:42:35 +0100 Subject: [PATCH 2/3] Fix lint --- pkg/utils/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/utils/api_test.go b/pkg/utils/api_test.go index ad7acb0b6..40fcb8f26 100644 --- a/pkg/utils/api_test.go +++ b/pkg/utils/api_test.go @@ -1,4 +1,4 @@ -package utils +package utils //nolint:revive //TODO: figure out a better name for this package import ( "testing" From 1dd2f6161397d1cfc1a7614222c413ddc1947ae4 Mon Sep 17 00:00:00 2001 From: Ksenia Bobrova Date: Tue, 3 Mar 2026 16:53:00 +0100 Subject: [PATCH 3/3] Fixed int conversion edge cases --- pkg/github/params.go | 91 ++++++++++++++++++++++++++++----------- pkg/github/params_test.go | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 26 deletions(-) diff --git a/pkg/github/params.go b/pkg/github/params.go index b839544a8..1b45d61bd 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -3,6 +3,7 @@ package github import ( "errors" "fmt" + "math" "strconv" "github.com/google/go-github/v82/github" @@ -39,21 +40,64 @@ func isAcceptedError(err error) bool { return errors.As(err, &acceptedError) } -// toFloat64 attempts to convert a value to float64, handling both numeric and -// string representations. Some MCP clients send numeric values as strings. -func toFloat64(val any) (float64, bool) { +// toInt converts a value to int, handling both float64 and string representations. +// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf, +// fractional values, and values outside the int range. +func toInt(val any) (int, error) { + var f float64 switch v := val.(type) { case float64: - return v, true + f = v case string: - f, err := strconv.ParseFloat(v, 64) + var err error + f, err = strconv.ParseFloat(v, 64) if err != nil { - return 0, false + return 0, fmt.Errorf("invalid numeric value: %s", v) } - return f, true default: - return 0, false + return 0, fmt.Errorf("expected number, got %T", val) } + if math.IsNaN(f) || math.IsInf(f, 0) { + return 0, fmt.Errorf("non-finite numeric value") + } + if f != math.Trunc(f) { + return 0, fmt.Errorf("non-integer numeric value: %v", f) + } + if f > math.MaxInt || f < math.MinInt { + return 0, fmt.Errorf("numeric value out of int range: %v", f) + } + return int(f), nil +} + +// toInt64 converts a value to int64, handling both float64 and string representations. +// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf, +// fractional values, and values that lose precision in the float64→int64 conversion. +func toInt64(val any) (int64, error) { + var f float64 + switch v := val.(type) { + case float64: + f = v + case string: + var err error + f, err = strconv.ParseFloat(v, 64) + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + default: + return 0, fmt.Errorf("expected number, got %T", val) + } + if math.IsNaN(f) || math.IsInf(f, 0) { + return 0, fmt.Errorf("non-finite numeric value") + } + if f != math.Trunc(f) { + return 0, fmt.Errorf("non-integer numeric value: %v", f) + } + result := int64(f) + // Check round-trip to detect precision loss for large int64 values + if float64(result) != f { + return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f) + } + return result, nil } // RequiredParam is a helper function that can be used to fetch a requested parameter from the request. @@ -93,16 +137,16 @@ func RequiredInt(args map[string]any, p string) (int, error) { return 0, fmt.Errorf("missing required parameter: %s", p) } - f, ok := toFloat64(v) - if !ok { - return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, v) + result, err := toInt(v) + if err != nil { + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) } - if f == 0 { + if result == 0 { return 0, fmt.Errorf("missing required parameter: %s", p) } - return int(f), nil + return result, nil } // RequiredBigInt is a helper function that can be used to fetch a requested parameter from the request. @@ -117,20 +161,15 @@ func RequiredBigInt(args map[string]any, p string) (int64, error) { return 0, fmt.Errorf("missing required parameter: %s", p) } - f, ok := toFloat64(val) - if !ok { - return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, val) + result, err := toInt64(val) + if err != nil { + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) } - if f == 0 { + if result == 0 { return 0, fmt.Errorf("missing required parameter: %s", p) } - result := int64(f) - // Check if converting back produces the same value to avoid silent truncation - if float64(result) != f { - return 0, fmt.Errorf("parameter %s value %f is too large to fit in int64", p, f) - } return result, nil } @@ -164,12 +203,12 @@ func OptionalIntParam(args map[string]any, p string) (int, error) { return 0, nil } - f, ok := toFloat64(val) - if !ok { - return 0, fmt.Errorf("parameter %s is not a valid number, is %T", p, val) + result, err := toInt(val) + if err != nil { + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) } - return int(f), nil + return result, nil } // OptionalIntParamWithDefault is a helper function that can be used to fetch a requested parameter from the request diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index d75850b2e..2254b737e 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -2,6 +2,7 @@ package github import ( "fmt" + "math" "testing" "github.com/google/go-github/v82/github" @@ -198,6 +199,62 @@ func Test_RequiredInt(t *testing.T) { expected: 0, expectError: true, }, + { + name: "NaN string", + params: map[string]any{"count": "NaN"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "Inf string", + params: map[string]any{"count": "Inf"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "negative Inf string", + params: map[string]any{"count": "-Inf"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional string", + params: map[string]any{"count": "1.5"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional float64", + params: map[string]any{"count": float64(1.5)}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "NaN float64", + params: map[string]any{"count": math.NaN()}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "Inf float64", + params: map[string]any{"count": math.Inf(1)}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "MaxFloat64", + params: map[string]any{"count": math.MaxFloat64}, + paramName: "count", + expected: 0, + expectError: true, + }, } for _, tc := range tests { @@ -263,6 +320,27 @@ func Test_OptionalIntParam(t *testing.T) { expected: 0, expectError: true, }, + { + name: "NaN string", + params: map[string]any{"count": "NaN"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional string", + params: map[string]any{"count": "1.5"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional float64", + params: map[string]any{"count": float64(1.5)}, + paramName: "count", + expected: 0, + expectError: true, + }, } for _, tc := range tests {