Skip to content

Commit b50a343

Browse files
almaleksiaalmaleksia
authored
Gracefully handle numeric parameters passed as strings (#2130)
* Gracefully handle numeric parameters passed as strings
1 parent 3fe6bc0 commit b50a343

File tree

9 files changed

+571
-21
lines changed

9 files changed

+571
-21
lines changed

‎pkg/github/copilot.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server
209209
BaseRef string `mapstructure:"base_ref"`
210210
CustomInstructions string `mapstructure:"custom_instructions"`
211211
}
212-
if err := mapstructure.Decode(args, &params); err != nil {
212+
if err := mapstructure.WeakDecode(args, &params); err != nil {
213213
return utils.NewToolResultError(err.Error()), nil, nil
214214
}
215215

‎pkg/github/copilot_test.go‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,115 @@ func TestAssignCopilotToIssue(t *testing.T) {
165165
),
166166
),
167167
},
168+
{
169+
name: "successful assignment with string issue_number",
170+
requestArgs: map[string]any{
171+
"owner": "owner",
172+
"repo": "repo",
173+
"issue_number": "123", // Some MCP clients send numeric values as strings
174+
},
175+
mockedClient: githubv4mock.NewMockedHTTPClient(
176+
githubv4mock.NewQueryMatcher(
177+
struct {
178+
Repository struct {
179+
SuggestedActors struct {
180+
Nodes []struct {
181+
Bot struct {
182+
ID githubv4.ID
183+
Login githubv4.String
184+
TypeName string `graphql:"__typename"`
185+
} `graphql:"... on Bot"`
186+
}
187+
PageInfo struct {
188+
HasNextPage bool
189+
EndCursor string
190+
}
191+
} `graphql:"suggestedActors(first: 100, after: $endCursor, capabilities: CAN_BE_ASSIGNED)"`
192+
} `graphql:"repository(owner: $owner, name: $name)"`
193+
}{},
194+
map[string]any{
195+
"owner": githubv4.String("owner"),
196+
"name": githubv4.String("repo"),
197+
"endCursor": (*githubv4.String)(nil),
198+
},
199+
githubv4mock.DataResponse(map[string]any{
200+
"repository": map[string]any{
201+
"suggestedActors": map[string]any{
202+
"nodes": []any{
203+
map[string]any{
204+
"id": githubv4.ID("copilot-swe-agent-id"),
205+
"login": githubv4.String("copilot-swe-agent"),
206+
"__typename": "Bot",
207+
},
208+
},
209+
},
210+
},
211+
}),
212+
),
213+
githubv4mock.NewQueryMatcher(
214+
struct {
215+
Repository struct {
216+
ID githubv4.ID
217+
Issue struct {
218+
ID githubv4.ID
219+
Assignees struct {
220+
Nodes []struct {
221+
ID githubv4.ID
222+
}
223+
} `graphql:"assignees(first: 100)"`
224+
} `graphql:"issue(number: $number)"`
225+
} `graphql:"repository(owner: $owner, name: $name)"`
226+
}{},
227+
map[string]any{
228+
"owner": githubv4.String("owner"),
229+
"name": githubv4.String("repo"),
230+
"number": githubv4.Int(123),
231+
},
232+
githubv4mock.DataResponse(map[string]any{
233+
"repository": map[string]any{
234+
"id": githubv4.ID("test-repo-id"),
235+
"issue": map[string]any{
236+
"id": githubv4.ID("test-issue-id"),
237+
"assignees": map[string]any{
238+
"nodes": []any{},
239+
},
240+
},
241+
},
242+
}),
243+
),
244+
githubv4mock.NewMutationMatcher(
245+
struct {
246+
UpdateIssue struct {
247+
Issue struct {
248+
ID githubv4.ID
249+
Number githubv4.Int
250+
URL githubv4.String
251+
}
252+
} `graphql:"updateIssue(input: $input)"`
253+
}{},
254+
UpdateIssueInput{
255+
ID: githubv4.ID("test-issue-id"),
256+
AssigneeIDs: []githubv4.ID{githubv4.ID("copilot-swe-agent-id")},
257+
AgentAssignment: &AgentAssignmentInput{
258+
BaseRef: nil,
259+
CustomAgent: ptrGitHubv4String(""),
260+
CustomInstructions: ptrGitHubv4String(""),
261+
TargetRepositoryID: githubv4.ID("test-repo-id"),
262+
},
263+
},
264+
nil,
265+
githubv4mock.DataResponse(map[string]any{
266+
"updateIssue": map[string]any{
267+
"issue": map[string]any{
268+
"id": githubv4.ID("test-issue-id"),
269+
"number": githubv4.Int(123),
270+
"url": githubv4.String("https://github.com/owner/repo/issues/123"),
271+
},
272+
},
273+
}),
274+
),
275+
),
276+
},
168277
{
169278
name: "successful assignment when there are existing assignees",
170279
requestArgs: map[string]any{

‎pkg/github/discussions.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
313313
Repo string
314314
DiscussionNumber int32
315315
}
316-
if err := mapstructure.Decode(args, &params); err != nil {
316+
if err := mapstructure.WeakDecode(args, &params); err != nil {
317317
return utils.NewToolResultError(err.Error()), nil, nil
318318
}
319319
client, err := deps.GetGQLClient(ctx)
@@ -417,7 +417,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
417417
Repo string
418418
DiscussionNumber int32
419419
}
420-
if err := mapstructure.Decode(args, &params); err != nil {
420+
if err := mapstructure.WeakDecode(args, &params); err != nil {
421421
return utils.NewToolResultError(err.Error()), nil, nil
422422
}
423423

‎pkg/github/discussions_test.go‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,50 @@ func Test_GetDiscussion(t *testing.T) {
590590
}
591591
}
592592

593+
func Test_GetDiscussionWithStringNumber(t *testing.T) {
594+
// Test that WeakDecode handles string discussionNumber from MCP clients
595+
toolDef := GetDiscussion(translations.NullTranslationHelper)
596+
597+
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}}}}"
598+
599+
vars := map[string]any{
600+
"owner": "owner",
601+
"repo": "repo",
602+
"discussionNumber": float64(1),
603+
}
604+
605+
matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, githubv4mock.DataResponse(map[string]any{
606+
"repository": map[string]any{"discussion": map[string]any{
607+
"number": 1,
608+
"title": "Test Discussion Title",
609+
"body": "This is a test discussion",
610+
"url": "https://github.com/owner/repo/discussions/1",
611+
"createdAt": "2025-04-25T12:00:00Z",
612+
"closed": false,
613+
"isAnswered": false,
614+
"category": map[string]any{"name": "General"},
615+
}},
616+
}))
617+
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
618+
gqlClient := githubv4.NewClient(httpClient)
619+
deps := BaseDeps{GQLClient: gqlClient}
620+
handler := toolDef.Handler(deps)
621+
622+
// Send discussionNumber as a string instead of a number
623+
reqParams := map[string]any{"owner": "owner", "repo": "repo", "discussionNumber": "1"}
624+
req := createMCPRequest(reqParams)
625+
res, err := handler(ContextWithDeps(context.Background(), deps), &req)
626+
require.NoError(t, err)
627+
628+
text := getTextResult(t, res).Text
629+
require.False(t, res.IsError, "expected no error, got: %s", text)
630+
631+
var out map[string]any
632+
require.NoError(t, json.Unmarshal([]byte(text), &out))
633+
assert.Equal(t, float64(1), out["number"])
634+
assert.Equal(t, "Test Discussion Title", out["title"])
635+
}
636+
593637
func Test_GetDiscussionComments(t *testing.T) {
594638
// Verify tool definition and schema
595639
toolDef := GetDiscussionComments(translations.NullTranslationHelper)
@@ -675,6 +719,67 @@ func Test_GetDiscussionComments(t *testing.T) {
675719
}
676720
}
677721

722+
func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) {
723+
// Test that WeakDecode handles string discussionNumber from MCP clients
724+
toolDef := GetDiscussionComments(translations.NullTranslationHelper)
725+
726+
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}}}}"
727+
728+
vars := map[string]any{
729+
"owner": "owner",
730+
"repo": "repo",
731+
"discussionNumber": float64(1),
732+
"first": float64(30),
733+
"after": (*string)(nil),
734+
}
735+
736+
mockResponse := githubv4mock.DataResponse(map[string]any{
737+
"repository": map[string]any{
738+
"discussion": map[string]any{
739+
"comments": map[string]any{
740+
"nodes": []map[string]any{
741+
{"body": "First comment"},
742+
},
743+
"pageInfo": map[string]any{
744+
"hasNextPage": false,
745+
"hasPreviousPage": false,
746+
"startCursor": "",
747+
"endCursor": "",
748+
},
749+
"totalCount": 1,
750+
},
751+
},
752+
},
753+
})
754+
matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse)
755+
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
756+
gqlClient := githubv4.NewClient(httpClient)
757+
deps := BaseDeps{GQLClient: gqlClient}
758+
handler := toolDef.Handler(deps)
759+
760+
// Send discussionNumber as a string instead of a number
761+
reqParams := map[string]any{
762+
"owner": "owner",
763+
"repo": "repo",
764+
"discussionNumber": "1",
765+
}
766+
request := createMCPRequest(reqParams)
767+
768+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
769+
require.NoError(t, err)
770+
771+
textContent := getTextResult(t, result)
772+
require.False(t, result.IsError, "expected no error, got: %s", textContent.Text)
773+
774+
var out struct {
775+
Comments []map[string]any `json:"comments"`
776+
TotalCount int `json:"totalCount"`
777+
}
778+
require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out))
779+
assert.Len(t, out.Comments, 1)
780+
assert.Equal(t, "First comment", out.Comments[0]["body"])
781+
}
782+
678783
func Test_ListDiscussionCategories(t *testing.T) {
679784
toolDef := ListDiscussionCategories(translations.NullTranslationHelper)
680785
tool := toolDef.Tool

0 commit comments

Comments
 (0)