Gracefully handle numeric parameters passed as strings#2130
Conversation
| v, err := RequiredParam[float64](args, p) | ||
| if err != nil { | ||
| return 0, err | ||
| v, ok := args[p] |
There was a problem hiding this comment.
the most important change is here and in OptionalIntParam
There was a problem hiding this comment.
Pull request overview
This PR aims to make tool argument handling more tolerant of MCP clients that send numeric inputs as JSON strings by adding weak type coercion during parameter decoding and by extending numeric parameter helpers to parse numeric strings.
Changes:
- Switched several tool handlers from
mapstructure.Decodetomapstructure.WeakDecodeto allow string→number coercion during decode. - Added
toFloat64plus updated numeric param helpers (RequiredInt,RequiredBigInt,OptionalIntParam) to accept numeric strings. - Added unit tests covering numeric-string inputs for PR reviews, discussions, Copilot assignment, and param helpers.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/pullrequests.go | Uses mapstructure.WeakDecode for PR review-related handler param decoding. |
| pkg/github/discussions.go | Uses mapstructure.WeakDecode for discussion handler param decoding. |
| pkg/github/copilot.go | Uses mapstructure.WeakDecode for Copilot assignment handler param decoding. |
| pkg/github/params.go | Adds numeric string parsing helper and updates int parsing helpers. |
| pkg/github/pullrequests_test.go | Adds tests covering string-encoded numeric inputs for PR review tools. |
| pkg/github/discussions_test.go | Adds tests covering string-encoded numeric inputs for discussion tools. |
| pkg/github/copilot_test.go | Adds tests covering string-encoded numeric inputs for Copilot assignment. |
| pkg/github/params_test.go | Adds tests for numeric-string inputs in param helpers and pagination. |
Comments suppressed due to low confidence (2)
pkg/github/discussions.go:421
- Same concern here: changing to
WeakDecodewon't help if argument validation occurs before handler execution. Since the tool'sInputSchemastill usestype: numberfordiscussionNumber(and pagination params viaWithCursorPagination), clients sending strings will still fail validation unless the schema or pre-validation layer is updated.
if err := mapstructure.WeakDecode(args, ¶ms); err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
pkg/github/pullrequests.go:1909
- Same as above:
WeakDecodehelps only after the handler starts. If schema validation happens before handler execution, string values forpullNumber/line/startLinewill still fail because the tool input schema continues to declare these astype: number. Consider updating the schema (e.g., allow string-or-number) or adding coercion before validation.
if err := mapstructure.WeakDecode(args, ¶ms); err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
|
In a lot of ways I find this ridiculous, the agents get the schema and clients could also coerce values before sending across the wire, but practically I think it's fine to just be less strict. Thanks for addressing this, I hope it increases success with a bunch of models. |
Summary
This PR changes prevent tools that accept numeric values from failing when number is passed as string.
Why
Fixes #2044
What changed
DecodetoWeakDecodeto do type coercion when numeric value is passed as string.Tested locally.
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs