Skip to content

Commit d15c2fb

Browse files
syf2211syf2211
committed
fix(issues): preserve native integer types in issue_number validation
Address review feedback on #2808: - Handle int/int64/uint/json.Number without float64 round-trip - Add RequiredBigInt regression tests for large int64 values - Add issue_read/issue_write/add_issue_comment tests for int issue_number - Assert invalid issue_number fails before API calls on write tools
1 parent 8c9eb4a commit d15c2fb

3 files changed

Lines changed: 242 additions & 57 deletions

File tree

‎pkg/github/issues_test.go‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,31 @@ func Test_GetIssue(t *testing.T) {
178178
},
179179
expectedIssue: mockIssue,
180180
},
181+
{
182+
name: "successful issue retrieval with int issue_number",
183+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
184+
GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue),
185+
}),
186+
requestArgs: map[string]any{
187+
"method": "get",
188+
"owner": "owner2",
189+
"repo": "repo2",
190+
"issue_number": int(42),
191+
},
192+
expectedIssue: mockIssue,
193+
},
194+
{
195+
name: "invalid issue_number does not call API",
196+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
197+
requestArgs: map[string]any{
198+
"method": "get",
199+
"owner": "owner2",
200+
"repo": "repo2",
201+
"issue_number": "not-a-number",
202+
},
203+
expectResultError: true,
204+
expectedErrMsg: "not a valid number",
205+
},
181206
{
182207
name: "issue not found",
183208
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -620,6 +645,18 @@ func Test_AddIssueComment(t *testing.T) {
620645
expectError: false,
621646
expectedComment: mockComment,
622647
},
648+
{
649+
name: "invalid issue_number does not call API",
650+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
651+
requestArgs: map[string]any{
652+
"owner": "owner",
653+
"repo": "repo",
654+
"issue_number": "not-a-number",
655+
"body": "This is a test comment",
656+
},
657+
expectError: false,
658+
expectedErrMsg: "not a valid number",
659+
},
623660
{
624661
name: "comment creation fails",
625662
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -2901,6 +2938,40 @@ func Test_UpdateIssue(t *testing.T) {
29012938
expectError: false,
29022939
expectedIssue: mockUpdatedIssue,
29032940
},
2941+
{
2942+
name: "partial update with int issue_number",
2943+
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2944+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
2945+
"title": "Updated Title",
2946+
}).andThen(
2947+
mockResponse(t, http.StatusOK, mockUpdatedIssue),
2948+
),
2949+
}),
2950+
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
2951+
requestArgs: map[string]any{
2952+
"method": "update",
2953+
"owner": "owner",
2954+
"repo": "repo",
2955+
"issue_number": int(123),
2956+
"title": "Updated Title",
2957+
},
2958+
expectError: false,
2959+
expectedIssue: mockUpdatedIssue,
2960+
},
2961+
{
2962+
name: "invalid issue_number does not call API",
2963+
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
2964+
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
2965+
requestArgs: map[string]any{
2966+
"method": "update",
2967+
"owner": "owner",
2968+
"repo": "repo",
2969+
"issue_number": "not-a-number",
2970+
"title": "Updated Title",
2971+
},
2972+
expectError: true,
2973+
expectedErrMsg: "not a valid number",
2974+
},
29042975
{
29052976
name: "partial update clears labels and assignees",
29062977
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{

‎pkg/github/params.go‎

Lines changed: 111 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -41,84 +41,138 @@ func isAcceptedError(err error) bool {
4141
return errors.As(err, &acceptedError)
4242
}
4343

44-
// numericToFloat64 normalizes numeric tool arguments to float64.
45-
// MCP clients may send JSON numbers as float64 (default json.Unmarshal),
46-
// native integer types (e.g. mcpcurl integer flags), or strings.
47-
func numericToFloat64(val any) (float64, error) {
44+
// float64ToInt64 converts a float64 to int64, rejecting non-finite, fractional,
45+
// and out-of-range values, including those that would lose precision.
46+
func float64ToInt64(f float64) (int64, error) {
47+
if math.IsNaN(f) || math.IsInf(f, 0) {
48+
return 0, fmt.Errorf("non-finite numeric value")
49+
}
50+
if f != math.Trunc(f) {
51+
return 0, fmt.Errorf("non-integer numeric value: %v", f)
52+
}
53+
if f > float64(math.MaxInt64) || f < float64(math.MinInt64) {
54+
return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f)
55+
}
56+
result := int64(f)
57+
if float64(result) != f {
58+
return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f)
59+
}
60+
return result, nil
61+
}
62+
63+
// float64ToInt converts a float64 to int, rejecting non-finite, fractional,
64+
// and out-of-range values.
65+
func float64ToInt(f float64) (int, error) {
66+
if math.IsNaN(f) || math.IsInf(f, 0) {
67+
return 0, fmt.Errorf("non-finite numeric value")
68+
}
69+
if f != math.Trunc(f) {
70+
return 0, fmt.Errorf("non-integer numeric value: %v", f)
71+
}
72+
if f > float64(math.MaxInt) || f < float64(math.MinInt) {
73+
return 0, fmt.Errorf("numeric value out of int range: %v", f)
74+
}
75+
return int(f), nil
76+
}
77+
78+
// toInt converts a value to int, handling float64, integer, and string representations.
79+
// Native integer types are preserved without a float64 round-trip so large values
80+
// are not corrupted. It rejects NaN, ±Inf, fractional values, and out-of-range values.
81+
func toInt(val any) (int, error) {
4882
switch v := val.(type) {
49-
case float64:
50-
return v, nil
51-
case float32:
52-
return float64(v), nil
5383
case int:
54-
return float64(v), nil
84+
return v, nil
5585
case int32:
56-
return float64(v), nil
86+
return int(v), nil
5787
case int64:
58-
return float64(v), nil
88+
if v > int64(math.MaxInt) || v < int64(math.MinInt) {
89+
return 0, fmt.Errorf("numeric value out of int range: %v", v)
90+
}
91+
return int(v), nil
5992
case uint:
60-
return float64(v), nil
93+
if v > uint(math.MaxInt) {
94+
return 0, fmt.Errorf("numeric value out of int range: %v", v)
95+
}
96+
return int(v), nil
6197
case uint32:
62-
return float64(v), nil
98+
return int(v), nil
6399
case uint64:
64-
return float64(v), nil
100+
if v > uint64(math.MaxInt) {
101+
return 0, fmt.Errorf("numeric value out of int range: %v", v)
102+
}
103+
return int(v), nil
104+
case float64:
105+
return float64ToInt(v)
106+
case float32:
107+
return float64ToInt(float64(v))
65108
case string:
66-
f, err := strconv.ParseFloat(v, 64)
109+
i, err := strconv.ParseInt(v, 10, 0)
67110
if err != nil {
68111
return 0, fmt.Errorf("invalid numeric value: %s", v)
69112
}
70-
return f, nil
113+
return int(i), nil
71114
case json.Number:
115+
if i, err := v.Int64(); err == nil {
116+
if i > int64(math.MaxInt) || i < int64(math.MinInt) {
117+
return 0, fmt.Errorf("numeric value out of int range: %v", i)
118+
}
119+
return int(i), nil
120+
}
72121
f, err := v.Float64()
73122
if err != nil {
74123
return 0, fmt.Errorf("invalid numeric value: %s", v)
75124
}
76-
return f, nil
125+
return float64ToInt(f)
77126
default:
78127
return 0, fmt.Errorf("expected number, got %T", val)
79128
}
80129
}
81130

82-
// toInt converts a value to int, handling float64, integer, and string representations.
83-
// Some MCP clients send numeric values as strings or native integer types. It rejects
84-
// NaN, ±Inf, fractional values, and values outside the int range.
85-
func toInt(val any) (int, error) {
86-
f, err := numericToFloat64(val)
87-
if err != nil {
88-
return 0, err
89-
}
90-
if math.IsNaN(f) || math.IsInf(f, 0) {
91-
return 0, fmt.Errorf("non-finite numeric value")
92-
}
93-
if f != math.Trunc(f) {
94-
return 0, fmt.Errorf("non-integer numeric value: %v", f)
95-
}
96-
if f > math.MaxInt || f < math.MinInt {
97-
return 0, fmt.Errorf("numeric value out of int range: %v", f)
98-
}
99-
return int(f), nil
100-
}
101-
102131
// toInt64 converts a value to int64, handling float64, integer, and string representations.
103-
// Some MCP clients send numeric values as strings or native integer types. It rejects
104-
// NaN, ±Inf, fractional values, and values that lose precision in the float64→int64 conversion.
132+
// Native integer types are preserved without a float64 round-trip so large values
133+
// are not corrupted. It rejects NaN, ±Inf, fractional values, and out-of-range values.
105134
func toInt64(val any) (int64, error) {
106-
f, err := numericToFloat64(val)
107-
if err != nil {
108-
return 0, err
109-
}
110-
if math.IsNaN(f) || math.IsInf(f, 0) {
111-
return 0, fmt.Errorf("non-finite numeric value")
112-
}
113-
if f != math.Trunc(f) {
114-
return 0, fmt.Errorf("non-integer numeric value: %v", f)
115-
}
116-
result := int64(f)
117-
// Check round-trip to detect precision loss for large int64 values
118-
if float64(result) != f {
119-
return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f)
135+
switch v := val.(type) {
136+
case int:
137+
return int64(v), nil
138+
case int32:
139+
return int64(v), nil
140+
case int64:
141+
return v, nil
142+
case uint:
143+
if uint64(v) > uint64(math.MaxInt64) {
144+
return 0, fmt.Errorf("numeric value out of int64 range: %v", v)
145+
}
146+
return int64(v), nil
147+
case uint32:
148+
return int64(v), nil
149+
case uint64:
150+
if v > uint64(math.MaxInt64) {
151+
return 0, fmt.Errorf("numeric value out of int64 range: %v", v)
152+
}
153+
return int64(v), nil
154+
case float64:
155+
return float64ToInt64(v)
156+
case float32:
157+
return float64ToInt64(float64(v))
158+
case string:
159+
i, err := strconv.ParseInt(v, 10, 64)
160+
if err != nil {
161+
return 0, fmt.Errorf("invalid numeric value: %s", v)
162+
}
163+
return i, nil
164+
case json.Number:
165+
if i, err := v.Int64(); err == nil {
166+
return i, nil
167+
}
168+
f, err := v.Float64()
169+
if err != nil {
170+
return 0, fmt.Errorf("invalid numeric value: %s", v)
171+
}
172+
return float64ToInt64(f)
173+
default:
174+
return 0, fmt.Errorf("expected number, got %T", val)
120175
}
121-
return result, nil
122176
}
123177

124178
// RequiredParam is a helper function that can be used to fetch a requested parameter from the request.
@@ -150,7 +204,7 @@ func RequiredParam[T comparable](args map[string]any, p string) (T, error) {
150204
// RequiredInt is a helper function that can be used to fetch a requested parameter from the request.
151205
// It does the following checks:
152206
// 1. Checks if the parameter is present in the request.
153-
// 2. Checks if the parameter is of the expected type (float64 or numeric string).
207+
// 2. Checks if the parameter is a valid integer (float64, native integer, or numeric string).
154208
// 3. Checks if the parameter is not empty, i.e: non-zero value
155209
func RequiredInt(args map[string]any, p string) (int, error) {
156210
v, ok := args[p]
@@ -173,7 +227,7 @@ func RequiredInt(args map[string]any, p string) (int, error) {
173227
// RequiredBigInt is a helper function that can be used to fetch a requested parameter from the request.
174228
// It does the following checks:
175229
// 1. Checks if the parameter is present in the request.
176-
// 2. Checks if the parameter is of the expected type (float64 or numeric string).
230+
// 2. Checks if the parameter is a valid integer (float64, native integer, or numeric string).
177231
// 3. Checks if the parameter is not empty, i.e: non-zero value.
178232
// 4. Validates that the float64 value can be safely converted to int64 without truncation.
179233
func RequiredBigInt(args map[string]any, p string) (int64, error) {
@@ -217,7 +271,7 @@ func OptionalParam[T any](args map[string]any, p string) (T, error) {
217271
// OptionalIntParam is a helper function that can be used to fetch a requested parameter from the request.
218272
// It does the following checks:
219273
// 1. Checks if the parameter is present in the request, if not, it returns its zero-value
220-
// 2. If it is present, it checks if the parameter is of the expected type (float64 or numeric string) and returns it
274+
// 2. If it is present, it checks if the parameter is a valid integer (float64, native integer, or numeric string) and returns it
221275
func OptionalIntParam(args map[string]any, p string) (int, error) {
222276
val, ok := args[p]
223277
if !ok {

‎pkg/github/params_test.go‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package github
22

33
import (
4+
"encoding/json"
45
"fmt"
56
"math"
67
"testing"
@@ -185,6 +186,20 @@ func Test_RequiredInt(t *testing.T) {
185186
expected: 42,
186187
expectError: false,
187188
},
189+
{
190+
name: "valid json.Number parameter",
191+
params: map[string]any{"count": json.Number("42")},
192+
paramName: "count",
193+
expected: 42,
194+
expectError: false,
195+
},
196+
{
197+
name: "valid uint parameter",
198+
params: map[string]any{"count": uint(42)},
199+
paramName: "count",
200+
expected: 42,
201+
expectError: false,
202+
},
188203
{
189204
name: "missing parameter",
190205
params: map[string]any{},
@@ -284,6 +299,51 @@ func Test_RequiredInt(t *testing.T) {
284299
})
285300
}
286301
}
302+
303+
func Test_RequiredBigInt(t *testing.T) {
304+
tests := []struct {
305+
name string
306+
params map[string]any
307+
paramName string
308+
expected int64
309+
expectError bool
310+
}{
311+
{
312+
name: "valid int64 parameter without float64 precision loss",
313+
params: map[string]any{"count": int64(9007199254740993)},
314+
paramName: "count",
315+
expected: 9007199254740993,
316+
expectError: false,
317+
},
318+
{
319+
name: "valid json.Number parameter",
320+
params: map[string]any{"count": json.Number("9007199254740993")},
321+
paramName: "count",
322+
expected: 9007199254740993,
323+
expectError: false,
324+
},
325+
{
326+
name: "valid uint64 parameter",
327+
params: map[string]any{"count": uint64(42)},
328+
paramName: "count",
329+
expected: 42,
330+
expectError: false,
331+
},
332+
}
333+
334+
for _, tc := range tests {
335+
t.Run(tc.name, func(t *testing.T) {
336+
result, err := RequiredBigInt(tc.params, tc.paramName)
337+
338+
if tc.expectError {
339+
assert.Error(t, err)
340+
} else {
341+
assert.NoError(t, err)
342+
assert.Equal(t, tc.expected, result)
343+
}
344+
})
345+
}
346+
}
287347
func Test_OptionalIntParam(t *testing.T) {
288348
tests := []struct {
289349
name string

0 commit comments

Comments
 (0)