From 505c6ec063220c9e17f0ae54b61b77d442ae8360 Mon Sep 17 00:00:00 2001 From: syf2211 Date: Wed, 8 Jul 2026 18:07:33 +0000 Subject: [PATCH] fix(issues): rebase native integer issue_number validation onto main Rebase #2808 onto upstream/main after hierarchy enrichment landed. Preserve int/int64/json.Number coercion without float64 round-trip and add regression tests for issue_read, issue_write, and add_issue_comment. Fixes #2807 --- pkg/github/deprecated_tool_aliases.go | 5 +- pkg/github/issues_test.go | 82 ++++++++++++++ pkg/github/params.go | 153 +++++++++++++++++++------- pkg/github/params_test.go | 74 +++++++++++++ 4 files changed, 274 insertions(+), 40 deletions(-) diff --git a/pkg/github/deprecated_tool_aliases.go b/pkg/github/deprecated_tool_aliases.go index 4415731fbe..c3ac325613 100644 --- a/pkg/github/deprecated_tool_aliases.go +++ b/pkg/github/deprecated_tool_aliases.go @@ -10,7 +10,10 @@ package github // "get_issue": "issue_read", // "create_pr": "pull_request_create", var DeprecatedToolAliases = map[string]string{ - // Add entries as tools are renamed + // Issues tools consolidated (#1211) + "get_issue": "issue_read", + "update_issue": "issue_write", + // Actions tools consolidated "list_workflows": "actions_list", "list_workflow_runs": "actions_list", diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index dbd246af94..11c6d47f05 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -191,6 +191,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{ @@ -2943,6 +2968,40 @@ func Test_UpdateIssue(t *testing.T) { expectError: false, expectedIssue: mockUpdatedIssue, }, + { + name: "partial update with int issue_number", + mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "title": "Updated Title", + }).andThen( + mockResponse(t, http.StatusOK, mockUpdatedIssue), + ), + }), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": int(123), + "title": "Updated Title", + }, + expectError: false, + expectedIssue: mockUpdatedIssue, + }, + { + name: "invalid issue_number does not call API", + mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": "not-a-number", + "title": "Updated Title", + }, + expectError: true, + expectedErrMsg: "not a valid number", + }, { name: "partial update clears labels and assignees", mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -4445,6 +4504,29 @@ func TestAddIssueComment(t *testing.T) { "body": "This is a comment", }, }, + { + name: "successful comment on issue with int issue_number", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusCreated, mockComment), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": int(42), + "body": "This is a comment", + }, + }, + { + name: "invalid issue_number does not call API", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": "not-a-number", + "body": "This is a comment", + }, + expectToolError: true, + expectedToolErrMsg: "not a valid number", + }, { name: "successful reaction to issue", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ diff --git a/pkg/github/params.go b/pkg/github/params.go index 9be51b94b9..260947f6e1 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -1,6 +1,7 @@ package github import ( + "encoding/json" "errors" "fmt" "math" @@ -40,64 +41,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. @@ -129,7 +204,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] @@ -152,7 +227,7 @@ func RequiredInt(args map[string]any, p string) (int, error) { // 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 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. // 4. Validates that the float64 value can be safely converted to int64 without truncation. func RequiredBigInt(args map[string]any, p string) (int64, error) { @@ -196,7 +271,7 @@ 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 (float64 or numeric string) and returns it +// 2. If it is present, it checks if the parameter is a valid integer (float64, native integer, or numeric string) and returns it func OptionalIntParam(args map[string]any, p string) (int, error) { val, ok := args[p] if !ok { diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index cbac37fee5..bd0bfb326d 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" @@ -171,6 +172,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{}, @@ -270,6 +299,51 @@ 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