Skip to content

Commit 1ebad59

Browse files
syf2211syf2211
committed
fix(discussions): validate required params in read handlers
Return explicit missing-parameter errors from get_discussion and get_discussion_comments instead of silently zero-filling inputs and issuing confusing GraphQL failures. Fixes #2740
1 parent c36e4e4 commit 1ebad59

2 files changed

Lines changed: 107 additions & 23 deletions

File tree

‎pkg/github/discussions.go‎

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"github.com/github/github-mcp-server/pkg/scopes"
1212
"github.com/github/github-mcp-server/pkg/translations"
1313
"github.com/github/github-mcp-server/pkg/utils"
14-
"github.com/go-viper/mapstructure/v2"
1514
"github.com/google/go-github/v89/github"
1615
"github.com/google/jsonschema-go/jsonschema"
1716
"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -313,15 +312,19 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
313312
},
314313
[]scopes.Scope{scopes.Repo},
315314
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
316-
// Decode params
317-
var params struct {
318-
Owner string
319-
Repo string
320-
DiscussionNumber int32
315+
owner, err := RequiredParam[string](args, "owner")
316+
if err != nil {
317+
return utils.NewToolResultError(err.Error()), nil, nil
321318
}
322-
if err := mapstructure.WeakDecode(args, &params); err != nil {
319+
repo, err := RequiredParam[string](args, "repo")
320+
if err != nil {
323321
return utils.NewToolResultError(err.Error()), nil, nil
324322
}
323+
discussionNumber, err := RequiredInt(args, "discussionNumber")
324+
if err != nil {
325+
return utils.NewToolResultError(err.Error()), nil, nil
326+
}
327+
325328
client, err := deps.GetGQLClient(ctx)
326329
if err != nil {
327330
return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil
@@ -345,9 +348,9 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
345348
} `graphql:"repository(owner: $owner, name: $repo)"`
346349
}
347350
vars := map[string]any{
348-
"owner": githubv4.String(params.Owner),
349-
"repo": githubv4.String(params.Repo),
350-
"discussionNumber": githubv4.Int(params.DiscussionNumber),
351+
"owner": githubv4.String(owner),
352+
"repo": githubv4.String(repo),
353+
"discussionNumber": githubv4.Int(discussionNumber),
351354
}
352355
if err := client.Query(ctx, &q, vars); err != nil {
353356
return utils.NewToolResultError(err.Error()), nil, nil
@@ -384,7 +387,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
384387
result := utils.NewToolResultText(string(out))
385388
// Discussion content is user-authored (untrusted); confidentiality
386389
// follows repo visibility.
387-
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent)
390+
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelRepoUserContent)
388391
return result, nil, nil
389392
},
390393
)
@@ -425,13 +428,16 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
425428
},
426429
[]scopes.Scope{scopes.Repo},
427430
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
428-
// Decode params
429-
var params struct {
430-
Owner string
431-
Repo string
432-
DiscussionNumber int32
431+
owner, err := RequiredParam[string](args, "owner")
432+
if err != nil {
433+
return utils.NewToolResultError(err.Error()), nil, nil
434+
}
435+
repo, err := RequiredParam[string](args, "repo")
436+
if err != nil {
437+
return utils.NewToolResultError(err.Error()), nil, nil
433438
}
434-
if err := mapstructure.WeakDecode(args, &params); err != nil {
439+
discussionNumber, err := RequiredInt(args, "discussionNumber")
440+
if err != nil {
435441
return utils.NewToolResultError(err.Error()), nil, nil
436442
}
437443

@@ -467,9 +473,9 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
467473
}
468474

469475
vars := map[string]any{
470-
"owner": githubv4.String(params.Owner),
471-
"repo": githubv4.String(params.Repo),
472-
"discussionNumber": githubv4.Int(params.DiscussionNumber),
476+
"owner": githubv4.String(owner),
477+
"repo": githubv4.String(repo),
478+
"discussionNumber": githubv4.Int(discussionNumber),
473479
"first": githubv4.Int(*paginationParams.First),
474480
}
475481
if paginationParams.After != nil {
@@ -592,7 +598,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
592598
result := utils.NewToolResultText(string(out))
593599
// Discussion comments are user-authored (untrusted); confidentiality
594600
// follows repo visibility.
595-
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent)
601+
result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelRepoUserContent)
596602
return result, nil, nil
597603
},
598604
)

‎pkg/github/discussions_test.go‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -590,8 +590,47 @@ func Test_GetDiscussion(t *testing.T) {
590590
}
591591
}
592592

593+
func Test_GetDiscussionRequiredParams(t *testing.T) {
594+
t.Parallel()
595+
596+
toolDef := GetDiscussion(translations.NullTranslationHelper)
597+
handler := toolDef.Handler(BaseDeps{GQLClient: githubv4.NewClient(githubv4mock.NewMockedHTTPClient())})
598+
599+
tests := []struct {
600+
name string
601+
requestArgs map[string]any
602+
expectedErrMsg string
603+
}{
604+
{
605+
name: "missing owner",
606+
requestArgs: map[string]any{"repo": "repo", "discussionNumber": float64(1)},
607+
expectedErrMsg: "missing required parameter: owner",
608+
},
609+
{
610+
name: "missing repo",
611+
requestArgs: map[string]any{"owner": "owner", "discussionNumber": float64(1)},
612+
expectedErrMsg: "missing required parameter: repo",
613+
},
614+
{
615+
name: "missing discussionNumber",
616+
requestArgs: map[string]any{"owner": "owner", "repo": "repo"},
617+
expectedErrMsg: "missing required parameter: discussionNumber",
618+
},
619+
}
620+
621+
for _, tc := range tests {
622+
t.Run(tc.name, func(t *testing.T) {
623+
req := createMCPRequest(tc.requestArgs)
624+
res, err := handler(ContextWithDeps(context.Background(), BaseDeps{}), &req)
625+
require.NoError(t, err)
626+
require.True(t, res.IsError)
627+
assert.Contains(t, getTextResult(t, res).Text, tc.expectedErrMsg)
628+
})
629+
}
630+
}
631+
593632
func Test_GetDiscussionWithStringNumber(t *testing.T) {
594-
// Test that WeakDecode handles string discussionNumber from MCP clients
633+
// Test that RequiredInt handles string discussionNumber from MCP clients
595634
toolDef := GetDiscussion(translations.NullTranslationHelper)
596635

597636
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}}}}"
@@ -723,8 +762,47 @@ func Test_GetDiscussionComments(t *testing.T) {
723762
assert.Equal(t, "This is the second comment", response.Comments[1].Body)
724763
}
725764

765+
func Test_GetDiscussionCommentsRequiredParams(t *testing.T) {
766+
t.Parallel()
767+
768+
toolDef := GetDiscussionComments(translations.NullTranslationHelper)
769+
handler := toolDef.Handler(BaseDeps{GQLClient: githubv4.NewClient(githubv4mock.NewMockedHTTPClient())})
770+
771+
tests := []struct {
772+
name string
773+
requestArgs map[string]any
774+
expectedErrMsg string
775+
}{
776+
{
777+
name: "missing owner",
778+
requestArgs: map[string]any{"repo": "repo", "discussionNumber": float64(1)},
779+
expectedErrMsg: "missing required parameter: owner",
780+
},
781+
{
782+
name: "missing repo",
783+
requestArgs: map[string]any{"owner": "owner", "discussionNumber": float64(1)},
784+
expectedErrMsg: "missing required parameter: repo",
785+
},
786+
{
787+
name: "missing discussionNumber",
788+
requestArgs: map[string]any{"owner": "owner", "repo": "repo"},
789+
expectedErrMsg: "missing required parameter: discussionNumber",
790+
},
791+
}
792+
793+
for _, tc := range tests {
794+
t.Run(tc.name, func(t *testing.T) {
795+
req := createMCPRequest(tc.requestArgs)
796+
res, err := handler(ContextWithDeps(context.Background(), BaseDeps{}), &req)
797+
require.NoError(t, err)
798+
require.True(t, res.IsError)
799+
assert.Contains(t, getTextResult(t, res).Text, tc.expectedErrMsg)
800+
})
801+
}
802+
}
803+
726804
func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) {
727-
// Test that WeakDecode handles string discussionNumber from MCP clients
805+
// Test that RequiredInt handles string discussionNumber from MCP clients
728806
toolDef := GetDiscussionComments(translations.NullTranslationHelper)
729807

730808
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{id,body,isAnswer},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}"

0 commit comments

Comments
 (0)