Skip to content

Commit 505c6ec

Browse files
syf2211syf2211
committed
fix(issues): rebase native integer issue_number validation onto main
Rebase github#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 github#2807
1 parent 778f5bb commit 505c6ec

4 files changed

Lines changed: 274 additions & 40 deletions

File tree

‎pkg/github/deprecated_tool_aliases.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ package github
1010
// "get_issue": "issue_read",
1111
// "create_pr": "pull_request_create",
1212
var DeprecatedToolAliases = map[string]string{
13-
// Add entries as tools are renamed
13+
// Issues tools consolidated (#1211)
14+
"get_issue": "issue_read",
15+
"update_issue": "issue_write",
16+
1417
// Actions tools consolidated
1518
"list_workflows": "actions_list",
1619
"list_workflow_runs": "actions_list",

‎pkg/github/issues_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,31 @@ func Test_GetIssue(t *testing.T) {
191191
},
192192
expectedIssue: mockIssue,
193193
},
194+
{
195+
name: "successful issue retrieval with int issue_number",
196+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
197+
GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue),
198+
}),
199+
requestArgs: map[string]any{
200+
"method": "get",
201+
"owner": "owner2",
202+
"repo": "repo2",
203+
"issue_number": int(42),
204+
},
205+
expectedIssue: mockIssue,
206+
},
207+
{
208+
name: "invalid issue_number does not call API",
209+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
210+
requestArgs: map[string]any{
211+
"method": "get",
212+
"owner": "owner2",
213+
"repo": "repo2",
214+
"issue_number": "not-a-number",
215+
},
216+
expectResultError: true,
217+
expectedErrMsg: "not a valid number",
218+
},
194219
{
195220
name: "issue not found",
196221
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -2943,6 +2968,40 @@ func Test_UpdateIssue(t *testing.T) {
29432968
expectError: false,
29442969
expectedIssue: mockUpdatedIssue,
29452970
},
2971+
{
2972+
name: "partial update with int issue_number",
2973+
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2974+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
2975+
"title": "Updated Title",
2976+
}).andThen(
2977+
mockResponse(t, http.StatusOK, mockUpdatedIssue),
2978+
),
2979+
}),
2980+
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
2981+
requestArgs: map[string]any{
2982+
"method": "update",
2983+
"owner": "owner",
2984+
"repo": "repo",
2985+
"issue_number": int(123),
2986+
"title": "Updated Title",
2987+
},
2988+
expectError: false,
2989+
expectedIssue: mockUpdatedIssue,
2990+
},
2991+
{
2992+
name: "invalid issue_number does not call API",
2993+
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
2994+
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
2995+
requestArgs: map[string]any{
2996+
"method": "update",
2997+
"owner": "owner",
2998+
"repo": "repo",
2999+
"issue_number": "not-a-number",
3000+
"title": "Updated Title",
3001+
},
3002+
expectError: true,
3003+
expectedErrMsg: "not a valid number",
3004+
},
29463005
{
29473006
name: "partial update clears labels and assignees",
29483007
mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -4445,6 +4504,29 @@ func TestAddIssueComment(t *testing.T) {
44454504
"body": "This is a comment",
44464505
},
44474506
},
4507+
{
4508+
name: "successful comment on issue with int issue_number",
4509+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
4510+
PostReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusCreated, mockComment),
4511+
}),
4512+
requestArgs: map[string]any{
4513+
"owner": "owner",
4514+
"repo": "repo",
4515+
"issue_number": int(42),
4516+
"body": "This is a comment",
4517+
},
4518+
},
4519+
{
4520+
name: "invalid issue_number does not call API",
4521+
requestArgs: map[string]any{
4522+
"owner": "owner",
4523+
"repo": "repo",
4524+
"issue_number": "not-a-number",
4525+
"body": "This is a comment",
4526+
},
4527+
expectToolError: true,
4528+
expectedToolErrMsg: "not a valid number",
4529+
},
44484530
{
44494531
name: "successful reaction to issue",
44504532
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{

‎pkg/github/params.go‎

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

33
import (
4+
"encoding/json"
45
"errors"
56
"fmt"
67
"math"
@@ -40,64 +41,138 @@ func isAcceptedError(err error) bool {
4041
return errors.As(err, &acceptedError)
4142
}
4243

43-
// toInt converts a value to int, handling both float64 and string representations.
44-
// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf,
45-
// fractional values, and values outside the int range.
46-
func toInt(val any) (int, error) {
47-
var f float64
48-
switch v := val.(type) {
49-
case float64:
50-
f = v
51-
case string:
52-
var err error
53-
f, err = strconv.ParseFloat(v, 64)
54-
if err != nil {
55-
return 0, fmt.Errorf("invalid numeric value: %s", v)
56-
}
57-
default:
58-
return 0, fmt.Errorf("expected number, got %T", val)
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)
5955
}
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) {
6066
if math.IsNaN(f) || math.IsInf(f, 0) {
6167
return 0, fmt.Errorf("non-finite numeric value")
6268
}
6369
if f != math.Trunc(f) {
6470
return 0, fmt.Errorf("non-integer numeric value: %v", f)
6571
}
66-
if f > math.MaxInt || f < math.MinInt {
72+
if f > float64(math.MaxInt) || f < float64(math.MinInt) {
6773
return 0, fmt.Errorf("numeric value out of int range: %v", f)
6874
}
6975
return int(f), nil
7076
}
7177

72-
// toInt64 converts a value to int64, handling both float64 and string representations.
73-
// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf,
74-
// fractional values, and values that lose precision in the float64→int64 conversion.
75-
func toInt64(val any) (int64, error) {
76-
var f float64
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) {
7782
switch v := val.(type) {
83+
case int:
84+
return v, nil
85+
case int32:
86+
return int(v), nil
87+
case int64:
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
92+
case uint:
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
97+
case uint32:
98+
return int(v), nil
99+
case uint64:
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
78104
case float64:
79-
f = v
105+
return float64ToInt(v)
106+
case float32:
107+
return float64ToInt(float64(v))
80108
case string:
81-
var err error
82-
f, err = strconv.ParseFloat(v, 64)
109+
i, err := strconv.ParseInt(v, 10, 0)
110+
if err != nil {
111+
return 0, fmt.Errorf("invalid numeric value: %s", v)
112+
}
113+
return int(i), nil
114+
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+
}
121+
f, err := v.Float64()
83122
if err != nil {
84123
return 0, fmt.Errorf("invalid numeric value: %s", v)
85124
}
125+
return float64ToInt(f)
86126
default:
87127
return 0, fmt.Errorf("expected number, got %T", val)
88128
}
89-
if math.IsNaN(f) || math.IsInf(f, 0) {
90-
return 0, fmt.Errorf("non-finite numeric value")
91-
}
92-
if f != math.Trunc(f) {
93-
return 0, fmt.Errorf("non-integer numeric value: %v", f)
94-
}
95-
result := int64(f)
96-
// Check round-trip to detect precision loss for large int64 values
97-
if float64(result) != f {
98-
return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f)
129+
}
130+
131+
// toInt64 converts a value to int64, handling float64, integer, and string representations.
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.
134+
func toInt64(val any) (int64, error) {
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)
99175
}
100-
return result, nil
101176
}
102177

103178
// 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) {
129204
// RequiredInt is a helper function that can be used to fetch a requested parameter from the request.
130205
// It does the following checks:
131206
// 1. Checks if the parameter is present in the request.
132-
// 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).
133208
// 3. Checks if the parameter is not empty, i.e: non-zero value
134209
func RequiredInt(args map[string]any, p string) (int, error) {
135210
v, ok := args[p]
@@ -152,7 +227,7 @@ func RequiredInt(args map[string]any, p string) (int, error) {
152227
// RequiredBigInt is a helper function that can be used to fetch a requested parameter from the request.
153228
// It does the following checks:
154229
// 1. Checks if the parameter is present in the request.
155-
// 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).
156231
// 3. Checks if the parameter is not empty, i.e: non-zero value.
157232
// 4. Validates that the float64 value can be safely converted to int64 without truncation.
158233
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) {
196271
// OptionalIntParam is a helper function that can be used to fetch a requested parameter from the request.
197272
// It does the following checks:
198273
// 1. Checks if the parameter is present in the request, if not, it returns its zero-value
199-
// 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
200275
func OptionalIntParam(args map[string]any, p string) (int, error) {
201276
val, ok := args[p]
202277
if !ok {

0 commit comments

Comments
 (0)