From c042498267bc2ec190971561ab82301d74fac0c5 Mon Sep 17 00:00:00 2001 From: harish-gaggar <64188979+harish-gaggar@users.noreply.github.com> Date: Sun, 4 Oct 2026 10:18:08 -0700 Subject: [PATCH] fix(params): accept native integer types for numeric tool parameters MCP clients sometimes send JSON integers as int or json.Number rather than float64, which caused RequiredInt to reject valid issue_number values before any GitHub API call. Fixes #2807 --- pkg/github/issues_test.go | 25 +++++++ pkg/github/params.go | 149 ++++++++++++++++++++++++++++---------- pkg/github/params_test.go | 75 +++++++++++++++++++ 3 files changed, 212 insertions(+), 37 deletions(-) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index f279b41243..199e622a95 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -200,6 +200,31 @@ func Test_GetIssue(t *testing.T) { }, expectedIssue: mockIssue, }, + { + name: "successful issue retrieval with int issue_number", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue), + }), + requestArgs: map[string]any{ + "method": "get", + "owner": "owner2", + "repo": "repo2", + "issue_number": int(42), + }, + expectedIssue: mockIssue, + }, + { + name: "invalid issue_number does not call API", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "method": "get", + "owner": "owner2", + "repo": "repo2", + "issue_number": "not-a-number", + }, + expectResultError: true, + expectedErrMsg: "not a valid number", + }, { name: "issue not found", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ diff --git a/pkg/github/params.go b/pkg/github/params.go index f4da2d6814..4af6f7ae13 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -1,6 +1,7 @@ package github import ( + "encoding/json" "errors" "fmt" "math" @@ -60,64 +61,138 @@ func isAcceptedError(err error) bool { return errors.As(err, &acceptedError) } -// 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: - 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) +// float64ToInt64 converts a float64 to int64, rejecting non-finite, fractional, +// and out-of-range values, including those that would lose precision. +func float64ToInt64(f float64) (int64, error) { + 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 > float64(math.MaxInt64) || f < float64(math.MinInt64) { + return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f) } + result := int64(f) + if float64(result) != f { + return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f) + } + return result, nil +} + +// float64ToInt converts a float64 to int, rejecting non-finite, fractional, +// and out-of-range values. +func float64ToInt(f float64) (int, error) { 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 { + if f > float64(math.MaxInt) || f < float64(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 +// toInt converts a value to int, handling float64, integer, and string representations. +// Native integer types are preserved without a float64 round-trip so large values +// are not corrupted. It rejects NaN, ±Inf, fractional values, and out-of-range values. +func toInt(val any) (int, error) { switch v := val.(type) { + case int: + return v, nil + case int32: + return int(v), nil + case int64: + if v > int64(math.MaxInt) || v < int64(math.MinInt) { + return 0, fmt.Errorf("numeric value out of int range: %v", v) + } + return int(v), nil + case uint: + if v > uint(math.MaxInt) { + return 0, fmt.Errorf("numeric value out of int range: %v", v) + } + return int(v), nil + case uint32: + return int(v), nil + case uint64: + if v > uint64(math.MaxInt) { + return 0, fmt.Errorf("numeric value out of int range: %v", v) + } + return int(v), nil case float64: - f = v + return float64ToInt(v) + case float32: + return float64ToInt(float64(v)) case string: - var err error - f, err = strconv.ParseFloat(v, 64) + i, err := strconv.ParseInt(v, 10, 0) if err != nil { return 0, fmt.Errorf("invalid numeric value: %s", v) } + return int(i), nil + case json.Number: + if i, err := v.Int64(); err == nil { + if i > int64(math.MaxInt) || i < int64(math.MinInt) { + return 0, fmt.Errorf("numeric value out of int range: %v", i) + } + return int(i), nil + } + f, err := v.Float64() + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + return float64ToInt(f) 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) +} + +// toInt64 converts a value to int64, handling float64, integer, and string representations. +// Native integer types are preserved without a float64 round-trip so large values +// are not corrupted. It rejects NaN, ±Inf, fractional values, and out-of-range values. +func toInt64(val any) (int64, error) { + switch v := val.(type) { + case int: + return int64(v), nil + case int32: + return int64(v), nil + case int64: + return v, nil + case uint: + if uint64(v) > uint64(math.MaxInt64) { + return 0, fmt.Errorf("numeric value out of int64 range: %v", v) + } + return int64(v), nil + case uint32: + return int64(v), nil + case uint64: + if v > uint64(math.MaxInt64) { + return 0, fmt.Errorf("numeric value out of int64 range: %v", v) + } + return int64(v), nil + case float64: + return float64ToInt64(v) + case float32: + return float64ToInt64(float64(v)) + case string: + i, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + return i, nil + case json.Number: + if i, err := v.Int64(); err == nil { + return i, nil + } + f, err := v.Float64() + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + return float64ToInt64(f) + default: + return 0, fmt.Errorf("expected number, got %T", val) } - return result, nil } // RequiredParam is a helper function that can be used to fetch a requested parameter from the request. @@ -149,7 +224,7 @@ 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 (float64 or numeric string). +// 2. Checks if the parameter is a valid integer (float64, native integer, 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, ok := args[p] diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index 3c0d7f43d1..a85ec0a20e 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -1,6 +1,7 @@ package github import ( + "encoding/json" "fmt" "math" "testing" @@ -206,6 +207,34 @@ func Test_RequiredInt(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid int parameter", + params: map[string]any{"count": int(42)}, + paramName: "count", + expected: 42, + expectError: false, + }, + { + name: "valid int64 parameter", + params: map[string]any{"count": int64(42)}, + paramName: "count", + expected: 42, + expectError: false, + }, + { + name: "valid json.Number parameter", + params: map[string]any{"count": json.Number("42")}, + paramName: "count", + expected: 42, + expectError: false, + }, + { + name: "valid uint parameter", + params: map[string]any{"count": uint(42)}, + paramName: "count", + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -305,6 +334,52 @@ func Test_RequiredInt(t *testing.T) { }) } } + +func Test_RequiredBigInt(t *testing.T) { + tests := []struct { + name string + params map[string]any + paramName string + expected int64 + expectError bool + }{ + { + name: "valid int64 parameter without float64 precision loss", + params: map[string]any{"count": int64(9007199254740993)}, + paramName: "count", + expected: 9007199254740993, + expectError: false, + }, + { + name: "valid json.Number parameter", + params: map[string]any{"count": json.Number("9007199254740993")}, + paramName: "count", + expected: 9007199254740993, + expectError: false, + }, + { + name: "valid uint64 parameter", + params: map[string]any{"count": uint64(42)}, + paramName: "count", + expected: 42, + expectError: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result, err := RequiredBigInt(tc.params, tc.paramName) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expected, result) + } + }) + } +} + func Test_OptionalIntParam(t *testing.T) { tests := []struct { name string