Skip to content

Gracefully handle numeric parameters passed as strings #2130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
almaleksia merged 3 commits into from
Mar 4, 2026
Merged

Gracefully handle numeric parameters passed as strings #2130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ef13f1
Gracefully handle numeric parameters passed as strings
almaleksia Mar 3, 2026
f785045
Fix lint
almaleksia Mar 3, 2026
1dd2f61
Fixed int conversion edge cases
almaleksia Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/github/copilot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server
BaseRef string `mapstructure:"base_ref"`
CustomInstructions string `mapstructure:"custom_instructions"`
}
if err := mapstructure.Decode(args, &params); err != nil {
if err := mapstructure.WeakDecode(args, &params); err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
}

Expand Down
109 changes: 109 additions & 0 deletions pkg/github/copilot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,115 @@ func TestAssignCopilotToIssue(t *testing.T) {
),
),
},
{
name: "successful assignment with string issue_number",
requestArgs: map[string]any{
"owner": "owner",
"repo": "repo",
"issue_number": "123", // Some MCP clients send numeric values as strings
},
mockedClient: githubv4mock.NewMockedHTTPClient(
githubv4mock.NewQueryMatcher(
struct {
Repository struct {
SuggestedActors struct {
Nodes []struct {
Bot struct {
ID githubv4.ID
Login githubv4.String
TypeName string `graphql:"__typename"`
} `graphql:"... on Bot"`
}
PageInfo struct {
HasNextPage bool
EndCursor string
}
} `graphql:"suggestedActors(first: 100, after: $endCursor, capabilities: CAN_BE_ASSIGNED)"`
} `graphql:"repository(owner: $owner, name: $name)"`
}{},
map[string]any{
"owner": githubv4.String("owner"),
"name": githubv4.String("repo"),
"endCursor": (*githubv4.String)(nil),
},
githubv4mock.DataResponse(map[string]any{
"repository": map[string]any{
"suggestedActors": map[string]any{
"nodes": []any{
map[string]any{
"id": githubv4.ID("copilot-swe-agent-id"),
"login": githubv4.String("copilot-swe-agent"),
"__typename": "Bot",
},
},
},
},
}),
),
githubv4mock.NewQueryMatcher(
struct {
Repository struct {
ID githubv4.ID
Issue struct {
ID githubv4.ID
Assignees struct {
Nodes []struct {
ID githubv4.ID
}
} `graphql:"assignees(first: 100)"`
} `graphql:"issue(number: $number)"`
} `graphql:"repository(owner: $owner, name: $name)"`
}{},
map[string]any{
"owner": githubv4.String("owner"),
"name": githubv4.String("repo"),
"number": githubv4.Int(123),
},
githubv4mock.DataResponse(map[string]any{
"repository": map[string]any{
"id": githubv4.ID("test-repo-id"),
"issue": map[string]any{
"id": githubv4.ID("test-issue-id"),
"assignees": map[string]any{
"nodes": []any{},
},
},
},
}),
),
githubv4mock.NewMutationMatcher(
struct {
UpdateIssue struct {
Issue struct {
ID githubv4.ID
Number githubv4.Int
URL githubv4.String
}
} `graphql:"updateIssue(input: $input)"`
}{},
UpdateIssueInput{
ID: githubv4.ID("test-issue-id"),
AssigneeIDs: []githubv4.ID{githubv4.ID("copilot-swe-agent-id")},
AgentAssignment: &AgentAssignmentInput{
BaseRef: nil,
CustomAgent: ptrGitHubv4String(""),
CustomInstructions: ptrGitHubv4String(""),
TargetRepositoryID: githubv4.ID("test-repo-id"),
},
},
nil,
githubv4mock.DataResponse(map[string]any{
"updateIssue": map[string]any{
"issue": map[string]any{
"id": githubv4.ID("test-issue-id"),
"number": githubv4.Int(123),
"url": githubv4.String("https://github.com/owner/repo/issues/123"),
},
},
}),
),
),
},
{
name: "successful assignment when there are existing assignees",
requestArgs: map[string]any{
Expand Down
4 changes: 2 additions & 2 deletions pkg/github/discussions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
Repo string
DiscussionNumber int32
}
if err := mapstructure.Decode(args, &params); err != nil {
if err := mapstructure.WeakDecode(args, &params); err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
}
client, err := deps.GetGQLClient(ctx)
Expand Down Expand Up @@ -417,7 +417,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
Repo string
DiscussionNumber int32
}
if err := mapstructure.Decode(args, &params); err != nil {
if err := mapstructure.WeakDecode(args, &params); err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
}

Expand Down
105 changes: 105 additions & 0 deletions pkg/github/discussions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,50 @@ func Test_GetDiscussion(t *testing.T) {
}
}

func Test_GetDiscussionWithStringNumber(t *testing.T) {
// Test that WeakDecode handles string discussionNumber from MCP clients
toolDef := GetDiscussion(translations.NullTranslationHelper)

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}}}}"

vars := map[string]any{
"owner": "owner",
"repo": "repo",
"discussionNumber": float64(1),
}

matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, githubv4mock.DataResponse(map[string]any{
"repository": map[string]any{"discussion": map[string]any{
"number": 1,
"title": "Test Discussion Title",
"body": "This is a test discussion",
"url": "https://github.com/owner/repo/discussions/1",
"createdAt": "2025-04-25T12:00:00Z",
"closed": false,
"isAnswered": false,
"category": map[string]any{"name": "General"},
}},
}))
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
gqlClient := githubv4.NewClient(httpClient)
deps := BaseDeps{GQLClient: gqlClient}
handler := toolDef.Handler(deps)

// Send discussionNumber as a string instead of a number
reqParams := map[string]any{"owner": "owner", "repo": "repo", "discussionNumber": "1"}
req := createMCPRequest(reqParams)
res, err := handler(ContextWithDeps(context.Background(), deps), &req)
require.NoError(t, err)

text := getTextResult(t, res).Text
require.False(t, res.IsError, "expected no error, got: %s", text)

var out map[string]any
require.NoError(t, json.Unmarshal([]byte(text), &out))
assert.Equal(t, float64(1), out["number"])
assert.Equal(t, "Test Discussion Title", out["title"])
}

func Test_GetDiscussionComments(t *testing.T) {
// Verify tool definition and schema
toolDef := GetDiscussionComments(translations.NullTranslationHelper)
Expand Down Expand Up @@ -675,6 +719,67 @@ func Test_GetDiscussionComments(t *testing.T) {
}
}

func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) {
// Test that WeakDecode handles string discussionNumber from MCP clients
toolDef := GetDiscussionComments(translations.NullTranslationHelper)

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}}}}"

vars := map[string]any{
"owner": "owner",
"repo": "repo",
"discussionNumber": float64(1),
"first": float64(30),
"after": (*string)(nil),
}

mockResponse := githubv4mock.DataResponse(map[string]any{
"repository": map[string]any{
"discussion": map[string]any{
"comments": map[string]any{
"nodes": []map[string]any{
{"body": "First comment"},
},
"pageInfo": map[string]any{
"hasNextPage": false,
"hasPreviousPage": false,
"startCursor": "",
"endCursor": "",
},
"totalCount": 1,
},
},
},
})
matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse)
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
gqlClient := githubv4.NewClient(httpClient)
deps := BaseDeps{GQLClient: gqlClient}
handler := toolDef.Handler(deps)

// Send discussionNumber as a string instead of a number
reqParams := map[string]any{
"owner": "owner",
"repo": "repo",
"discussionNumber": "1",
}
request := createMCPRequest(reqParams)

result, err := handler(ContextWithDeps(context.Background(), deps), &request)
require.NoError(t, err)

textContent := getTextResult(t, result)
require.False(t, result.IsError, "expected no error, got: %s", textContent.Text)

var out struct {
Comments []map[string]any `json:"comments"`
TotalCount int `json:"totalCount"`
}
require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out))
assert.Len(t, out.Comments, 1)
assert.Equal(t, "First comment", out.Comments[0]["body"])
}

func Test_ListDiscussionCategories(t *testing.T) {
toolDef := ListDiscussionCategories(translations.NullTranslationHelper)
tool := toolDef.Tool
Expand Down
Loading