From 2f9512070bde01f8e1b64eb0fc372b701afff046 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 11:55:47 +0100 Subject: [PATCH 01/26] initial pagination for `ListDiscussions` --- pkg/github/discussions.go | 72 ++++++++++++++++++++++---- pkg/github/discussions_test.go | 93 ++++++++++++++++++---------------- pkg/github/server.go | 85 +++++++++++++++++++++++++++++-- 3 files changed, 193 insertions(+), 57 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 3e53a633b..7cee253c1 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -31,6 +31,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp mcp.WithString("category", mcp.Description("Optional filter by discussion category ID. If provided, only discussions with this category are listed."), ), + WithGraphQLPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Required params @@ -49,6 +50,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } + // Get pagination parameters + pagination, err := OptionalGraphQLPaginationParams(request) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil @@ -61,7 +68,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp categoryID = &id } - // Now execute the discussions query + // Build GraphQL query arguments + // Use default of 30 if neither first nor last is specified + if pagination.First == nil && pagination.Last == nil { + defaultFirst := int32(30) + pagination.First = &defaultFirst + } + var discussions []*github.Discussion if categoryID != nil { // Query with category filter (server-side filtering) @@ -77,13 +90,21 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), "categoryId": *categoryID, + "first": (*githubv4.Int)(pagination.First), + "last": (*githubv4.Int)(pagination.Last), + "after": (*githubv4.String)(pagination.After), + "before": (*githubv4.String)(pagination.Before), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -102,6 +123,20 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } discussions = append(discussions, di) } + + // Include pagination info in response + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + }, + } + out, err := json.Marshal(response) + if err != nil { + return nil, fmt.Errorf("failed to marshal discussions: %w", err) + } + return mcp.NewToolResultText(string(out)), nil } else { // Query without category filter var query struct { @@ -116,12 +151,20 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100)"` + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"discussions(first: $first, last: $last, after: $after, before: $before)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "first": (*githubv4.Int)(pagination.First), + "last": (*githubv4.Int)(pagination.Last), + "after": (*githubv4.String)(pagination.After), + "before": (*githubv4.String)(pagination.Before), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -140,14 +183,21 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } discussions = append(discussions, di) } - } - // Marshal and return - out, err := json.Marshal(discussions) - if err != nil { - return nil, fmt.Errorf("failed to marshal discussions: %w", err) + // Include pagination info in response + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + }, + } + out, err := json.Marshal(response) + if err != nil { + return nil, fmt.Errorf("failed to marshal discussions: %w", err) + } + return mcp.NewToolResultText(string(out)), nil } - return mcp.NewToolResultText(string(out)), nil } } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 5132c6ce0..d6d59b4d4 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -27,12 +27,24 @@ var ( } mockResponseListAll = githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ - "discussions": map[string]any{"nodes": discussionsAll}, + "discussions": map[string]any{ + "nodes": discussionsAll, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, + }, }, }) mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ - "discussions": map[string]any{"nodes": discussionsGeneral}, + "discussions": map[string]any{ + "nodes": discussionsGeneral, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, + }, }, }) mockErrorRepoNotFound = githubv4mock.ErrorResponse("repository not found") @@ -48,54 +60,38 @@ func Test_ListDiscussions(t *testing.T) { assert.Contains(t, toolDef.InputSchema.Properties, "repo") assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) - // mock for the call to ListDiscussions without category filter - var qDiscussions struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + // Use exact string queries that match implementation output (from error messages) + qDiscussions := "query($after:String$before:String$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" - // mock for the call to get discussions with category filter - var qDiscussionsFiltered struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + qDiscussionsFiltered := "query($after:String$before:String$categoryId:ID!$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + // Variables matching what GraphQL receives after JSON marshaling/unmarshaling varsListAll := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), + "owner": "owner", + "repo": "repo", + "first": float64(30), + "last": nil, + "after": nil, + "before": nil, } varsRepoNotFound := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("nonexistent-repo"), + "owner": "owner", + "repo": "nonexistent-repo", + "first": float64(30), + "last": nil, + "after": nil, + "before": nil, } varsDiscussionsFiltered := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "categoryId": githubv4.ID("DIC_kwDOABC123"), + "owner": "owner", + "repo": "repo", + "categoryId": "DIC_kwDOABC123", + "first": float64(30), + "last": nil, + "after": nil, + "before": nil, } tests := []struct { @@ -167,10 +163,21 @@ func Test_ListDiscussions(t *testing.T) { } require.NoError(t, err) - var returnedDiscussions []*github.Discussion - err = json.Unmarshal([]byte(text), &returnedDiscussions) + // Debug: print the actual response + t.Logf("Actual response text: %q", text) + + // Parse the structured response with pagination info + var response struct { + Discussions []*github.Discussion `json:"discussions"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + } + err = json.Unmarshal([]byte(text), &response) require.NoError(t, err) + returnedDiscussions := response.Discussions assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions)) // Verify that all returned discussions have a category if filtered diff --git a/pkg/github/server.go b/pkg/github/server.go index e7b831791..d5fea224f 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -174,9 +174,7 @@ func OptionalStringArrayParam(r mcp.CallToolRequest, p string) ([]string, error) } } -// WithPagination returns a ToolOption that adds "page" and "perPage" parameters to the tool. -// The "page" parameter is optional, min 1. -// The "perPage" parameter is optional, min 1, max 100. If unset, defaults to 30. +// WithPagination adds REST API pagination parameters to a tool. // https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api func WithPagination() mcp.ToolOption { return func(tool *mcp.Tool) { @@ -193,6 +191,32 @@ func WithPagination() mcp.ToolOption { } } +// WithGraphQLPagination adds GraphQL cursor-based pagination parameters to a tool. +// https://docs.github.com/en/graphql/reference/objects#connection +func WithGraphQLPagination() mcp.ToolOption { + return func(tool *mcp.Tool) { + mcp.WithNumber("first", + mcp.Description("Number of items to return per page (min 1, max 100)"), + mcp.Min(1), + mcp.Max(100), + )(tool) + + mcp.WithNumber("last", + mcp.Description("Number of items to return from the end (min 1, max 100)"), + mcp.Min(1), + mcp.Max(100), + )(tool) + + mcp.WithString("after", + mcp.Description("Cursor for pagination, use the 'after' field from the previous response"), + )(tool) + + mcp.WithString("before", + mcp.Description("Cursor for pagination, use the 'before' field from the previous response"), + )(tool) + } +} + type PaginationParams struct { page int perPage int @@ -218,6 +242,61 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) { }, nil } +type GraphQLPaginationParams struct { + First *int32 + Last *int32 + After *string + Before *string +} + +// OptionalGraphQLPaginationParams returns the GraphQL cursor-based pagination parameters from the request. +// It validates that the parameters are used correctly according to GraphQL connection spec. +func OptionalGraphQLPaginationParams(r mcp.CallToolRequest) (GraphQLPaginationParams, error) { + var params GraphQLPaginationParams + + if val, err := OptionalParam[float64](r, "first"); err != nil { + return GraphQLPaginationParams{}, err + } else if val != 0 { + first := int32(val) + params.First = &first + } + + if val, err := OptionalParam[float64](r, "last"); err != nil { + return GraphQLPaginationParams{}, err + } else if val != 0 { + last := int32(val) + params.Last = &last + } + + if val, err := OptionalParam[string](r, "after"); err != nil { + return GraphQLPaginationParams{}, err + } else if val != "" { + params.After = &val + } + + if val, err := OptionalParam[string](r, "before"); err != nil { + return GraphQLPaginationParams{}, err + } else if val != "" { + params.Before = &val + } + + // Validate pagination parameters according to GraphQL connection spec + if params.First != nil && params.Last != nil { + return GraphQLPaginationParams{}, fmt.Errorf("only one of 'first' or 'last' may be specified") + } + if params.After != nil && params.Before != nil { + return GraphQLPaginationParams{}, fmt.Errorf("only one of 'after' or 'before' may be specified") + } + if params.After != nil && params.Last != nil { + return GraphQLPaginationParams{}, fmt.Errorf("'after' cannot be used with 'last'. Did you mean to use 'before' instead?") + } + if params.Before != nil && params.First != nil { + return GraphQLPaginationParams{}, fmt.Errorf("'before' cannot be used with 'first'. Did you mean to use 'after' instead?") + } + + return params, nil +} + func MarshalledTextResult(v any) *mcp.CallToolResult { data, err := json.Marshal(v) if err != nil { From d464ac13ba8f504399e924e54e385178f7b13335 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 12:04:16 +0100 Subject: [PATCH 02/26] redo category id var cast --- pkg/github/discussions_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index d6d59b4d4..5a1792910 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -67,8 +67,8 @@ func Test_ListDiscussions(t *testing.T) { // Variables matching what GraphQL receives after JSON marshaling/unmarshaling varsListAll := map[string]interface{}{ - "owner": "owner", - "repo": "repo", + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), "first": float64(30), "last": nil, "after": nil, @@ -76,8 +76,8 @@ func Test_ListDiscussions(t *testing.T) { } varsRepoNotFound := map[string]interface{}{ - "owner": "owner", - "repo": "nonexistent-repo", + "owner": githubv4.String("owner"), + "repo": githubv4.String("nonexistent-repo"), "first": float64(30), "last": nil, "after": nil, @@ -85,9 +85,9 @@ func Test_ListDiscussions(t *testing.T) { } varsDiscussionsFiltered := map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "categoryId": "DIC_kwDOABC123", + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "categoryId": githubv4.ID("DIC_kwDOABC123"), "first": float64(30), "last": nil, "after": nil, From d745f5c9b0cbbf479f9bf334336fc65456c9bc82 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 12:16:42 +0100 Subject: [PATCH 03/26] add GraphQL pagination support for discussion comments and categories --- pkg/github/discussions.go | 96 +++++++++++++++++++++------------- pkg/github/discussions_test.go | 77 +++++++++++++++------------ 2 files changed, 102 insertions(+), 71 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 7cee253c1..09ff4d468 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -286,6 +286,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati mcp.WithString("owner", mcp.Required(), mcp.Description("Repository owner")), mcp.WithString("repo", mcp.Required(), mcp.Description("Repository name")), mcp.WithNumber("discussionNumber", mcp.Required(), mcp.Description("Discussion Number")), + WithGraphQLPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -298,6 +299,17 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati return mcp.NewToolResultError(err.Error()), nil } + paginationParams, err := OptionalGraphQLPaginationParams(request) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + // Use default of 100 if neither first nor last is specified + if paginationParams.First == nil && paginationParams.Last == nil { + defaultFirst := int32(100) + paginationParams.First = &defaultFirst + } + client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil @@ -310,7 +322,11 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati Nodes []struct { Body githubv4.String } - } `graphql:"comments(first:100)"` + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"comments(first: $first, after: $after)"` } `graphql:"discussion(number: $discussionNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -318,16 +334,27 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), "discussionNumber": githubv4.Int(params.DiscussionNumber), + "first": (*githubv4.Int)(paginationParams.First), + "after": (*githubv4.String)(paginationParams.After), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } + var comments []*github.IssueComment for _, c := range q.Repository.Discussion.Comments.Nodes { comments = append(comments, &github.IssueComment{Body: github.Ptr(string(c.Body))}) } - out, err := json.Marshal(comments) + result := map[string]interface{}{ + "comments": comments, + "pageInfo": map[string]interface{}{ + "hasNextPage": bool(q.Repository.Discussion.Comments.PageInfo.HasNextPage), + "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), + }, + } + + out, err := json.Marshal(result) if err != nil { return nil, fmt.Errorf("failed to marshal comments: %w", err) } @@ -351,55 +378,34 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl mcp.Required(), mcp.Description("Repository name"), ), - mcp.WithNumber("first", - mcp.Description("Number of categories to return per page (min 1, max 100)"), - mcp.Min(1), - mcp.Max(100), - ), - mcp.WithNumber("last", - mcp.Description("Number of categories to return from the end (min 1, max 100)"), - mcp.Min(1), - mcp.Max(100), - ), - mcp.WithString("after", - mcp.Description("Cursor for pagination, use the 'after' field from the previous response"), - ), - mcp.WithString("before", - mcp.Description("Cursor for pagination, use the 'before' field from the previous response"), - ), + WithGraphQLPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params var params struct { - Owner string - Repo string - First int32 - Last int32 - After string - Before string + Owner string + Repo string } if err := mapstructure.Decode(request.Params.Arguments, ¶ms); err != nil { return mcp.NewToolResultError(err.Error()), nil } - // Validate pagination parameters - if params.First != 0 && params.Last != 0 { - return mcp.NewToolResultError("only one of 'first' or 'last' may be specified"), nil - } - if params.After != "" && params.Before != "" { - return mcp.NewToolResultError("only one of 'after' or 'before' may be specified"), nil - } - if params.After != "" && params.Last != 0 { - return mcp.NewToolResultError("'after' cannot be used with 'last'. Did you mean to use 'before' instead?"), nil + pagination, err := OptionalGraphQLPaginationParams(request) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil } - if params.Before != "" && params.First != 0 { - return mcp.NewToolResultError("'before' cannot be used with 'first'. Did you mean to use 'after' instead?"), nil + + // Use default of 100 if neither first nor last is specified + if pagination.First == nil && pagination.Last == nil { + defaultFirst := int32(100) + pagination.First = &defaultFirst } client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil } + var q struct { Repository struct { DiscussionCategories struct { @@ -407,16 +413,23 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl ID githubv4.ID Name githubv4.String } - } `graphql:"discussionCategories(first: 100)"` + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"discussionCategories(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), + "first": (*githubv4.Int)(pagination.First), + "after": (*githubv4.String)(pagination.After), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } + var categories []map[string]string for _, c := range q.Repository.DiscussionCategories.Nodes { categories = append(categories, map[string]string{ @@ -424,7 +437,16 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl "name": string(c.Name), }) } - out, err := json.Marshal(categories) + + result := map[string]interface{}{ + "categories": categories, + "pageInfo": map[string]interface{}{ + "hasNextPage": bool(q.Repository.DiscussionCategories.PageInfo.HasNextPage), + "endCursor": string(q.Repository.DiscussionCategories.PageInfo.EndCursor), + }, + } + + out, err := json.Marshal(result) if err != nil { return nil, fmt.Errorf("failed to marshal discussion categories: %w", err) } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 5a1792910..65e5bca6e 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -294,22 +294,18 @@ func Test_GetDiscussionComments(t *testing.T) { assert.Contains(t, toolDef.InputSchema.Properties, "discussionNumber") assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) - var q struct { - Repository struct { - Discussion struct { - Comments struct { - Nodes []struct { - Body githubv4.String - } - } `graphql:"comments(first:100)"` - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + // Use exact string query that matches implementation output + 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,endCursor}}}}}" + + // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ "owner": githubv4.String("owner"), "repo": githubv4.String("repo"), "discussionNumber": githubv4.Int(1), + "first": float64(100), // Default value when no pagination specified + "after": nil, } + mockResponse := githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "discussion": map[string]any{ @@ -318,11 +314,15 @@ func Test_GetDiscussionComments(t *testing.T) { {"body": "This is the first comment"}, {"body": "This is the second comment"}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, }, }) - matcher := githubv4mock.NewQueryMatcher(q, vars, mockResponse) + matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse) httpClient := githubv4mock.NewMockedHTTPClient(matcher) gqlClient := githubv4.NewClient(httpClient) _, handler := GetDiscussionComments(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) @@ -338,31 +338,32 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) - var returnedComments []*github.IssueComment - err = json.Unmarshal([]byte(textContent.Text), &returnedComments) + var response struct { + Comments []*github.IssueComment `json:"comments"` + PageInfo map[string]interface{} `json:"pageInfo"` + } + err = json.Unmarshal([]byte(textContent.Text), &response) require.NoError(t, err) - assert.Len(t, returnedComments, 2) + assert.Len(t, response.Comments, 2) expectedBodies := []string{"This is the first comment", "This is the second comment"} - for i, comment := range returnedComments { + for i, comment := range response.Comments { assert.Equal(t, expectedBodies[i], *comment.Body) } + assert.Equal(t, false, response.PageInfo["hasNextPage"]) } func Test_ListDiscussionCategories(t *testing.T) { - var q struct { - Repository struct { - DiscussionCategories struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"discussionCategories(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + // Use exact string query that matches implementation output + qListCategories := "query($after:String$first:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" + + // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ "owner": githubv4.String("owner"), "repo": githubv4.String("repo"), + "first": float64(100), // Default value when no pagination specified + "after": nil, } + mockResp := githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "discussionCategories": map[string]any{ @@ -370,10 +371,14 @@ func Test_ListDiscussionCategories(t *testing.T) { {"id": "123", "name": "CategoryOne"}, {"id": "456", "name": "CategoryTwo"}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, }) - matcher := githubv4mock.NewQueryMatcher(q, vars, mockResp) + matcher := githubv4mock.NewQueryMatcher(qListCategories, vars, mockResp) httpClient := githubv4mock.NewMockedHTTPClient(matcher) gqlClient := githubv4.NewClient(httpClient) @@ -389,11 +394,15 @@ func Test_ListDiscussionCategories(t *testing.T) { require.NoError(t, err) text := getTextResult(t, result).Text - var categories []map[string]string - require.NoError(t, json.Unmarshal([]byte(text), &categories)) - assert.Len(t, categories, 2) - assert.Equal(t, "123", categories[0]["id"]) - assert.Equal(t, "CategoryOne", categories[0]["name"]) - assert.Equal(t, "456", categories[1]["id"]) - assert.Equal(t, "CategoryTwo", categories[1]["name"]) + var response struct { + Categories []map[string]string `json:"categories"` + PageInfo map[string]interface{} `json:"pageInfo"` + } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Len(t, response.Categories, 2) + assert.Equal(t, "123", response.Categories[0]["id"]) + assert.Equal(t, "CategoryOne", response.Categories[0]["name"]) + assert.Equal(t, "456", response.Categories[1]["id"]) + assert.Equal(t, "CategoryTwo", response.Categories[1]["name"]) + assert.Equal(t, false, response.PageInfo["hasNextPage"]) } From 83c0a7446eae66dec1bc8eda1bc09d35459f4cd4 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 12:24:13 +0100 Subject: [PATCH 04/26] remove pageinfo returns --- pkg/github/discussions.go | 40 ++++--------------------------- pkg/github/discussions_test.go | 44 ++++++++++------------------------ 2 files changed, 17 insertions(+), 67 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 09ff4d468..3036b1796 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -124,15 +124,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - // Include pagination info in response - response := map[string]interface{}{ - "discussions": discussions, - "pageInfo": map[string]interface{}{ - "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, - "endCursor": query.Repository.Discussions.PageInfo.EndCursor, - }, - } - out, err := json.Marshal(response) + out, err := json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } @@ -184,15 +176,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - // Include pagination info in response - response := map[string]interface{}{ - "discussions": discussions, - "pageInfo": map[string]interface{}{ - "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, - "endCursor": query.Repository.Discussions.PageInfo.EndCursor, - }, - } - out, err := json.Marshal(response) + out, err := json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } @@ -346,15 +330,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati comments = append(comments, &github.IssueComment{Body: github.Ptr(string(c.Body))}) } - result := map[string]interface{}{ - "comments": comments, - "pageInfo": map[string]interface{}{ - "hasNextPage": bool(q.Repository.Discussion.Comments.PageInfo.HasNextPage), - "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), - }, - } - - out, err := json.Marshal(result) + out, err := json.Marshal(comments) if err != nil { return nil, fmt.Errorf("failed to marshal comments: %w", err) } @@ -438,15 +414,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl }) } - result := map[string]interface{}{ - "categories": categories, - "pageInfo": map[string]interface{}{ - "hasNextPage": bool(q.Repository.DiscussionCategories.PageInfo.HasNextPage), - "endCursor": string(q.Repository.DiscussionCategories.PageInfo.EndCursor), - }, - } - - out, err := json.Marshal(result) + out, err := json.Marshal(categories) if err != nil { return nil, fmt.Errorf("failed to marshal discussion categories: %w", err) } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 65e5bca6e..8d7a1fd60 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -163,21 +163,11 @@ func Test_ListDiscussions(t *testing.T) { } require.NoError(t, err) - // Debug: print the actual response - t.Logf("Actual response text: %q", text) - // Parse the structured response with pagination info - var response struct { - Discussions []*github.Discussion `json:"discussions"` - PageInfo struct { - HasNextPage bool `json:"hasNextPage"` - EndCursor string `json:"endCursor"` - } `json:"pageInfo"` - } - err = json.Unmarshal([]byte(text), &response) + var returnedDiscussions []*github.Discussion + err = json.Unmarshal([]byte(text), &returnedDiscussions) require.NoError(t, err) - returnedDiscussions := response.Discussions assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions)) // Verify that all returned discussions have a category if filtered @@ -338,18 +328,14 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) - var response struct { - Comments []*github.IssueComment `json:"comments"` - PageInfo map[string]interface{} `json:"pageInfo"` - } - err = json.Unmarshal([]byte(textContent.Text), &response) + var comments []*github.IssueComment + err = json.Unmarshal([]byte(textContent.Text), &comments) require.NoError(t, err) - assert.Len(t, response.Comments, 2) + assert.Len(t, comments, 2) expectedBodies := []string{"This is the first comment", "This is the second comment"} - for i, comment := range response.Comments { + for i, comment := range comments { assert.Equal(t, expectedBodies[i], *comment.Body) } - assert.Equal(t, false, response.PageInfo["hasNextPage"]) } func Test_ListDiscussionCategories(t *testing.T) { @@ -394,15 +380,11 @@ func Test_ListDiscussionCategories(t *testing.T) { require.NoError(t, err) text := getTextResult(t, result).Text - var response struct { - Categories []map[string]string `json:"categories"` - PageInfo map[string]interface{} `json:"pageInfo"` - } - require.NoError(t, json.Unmarshal([]byte(text), &response)) - assert.Len(t, response.Categories, 2) - assert.Equal(t, "123", response.Categories[0]["id"]) - assert.Equal(t, "CategoryOne", response.Categories[0]["name"]) - assert.Equal(t, "456", response.Categories[1]["id"]) - assert.Equal(t, "CategoryTwo", response.Categories[1]["name"]) - assert.Equal(t, false, response.PageInfo["hasNextPage"]) + var categories []map[string]string + require.NoError(t, json.Unmarshal([]byte(text), &categories)) + assert.Len(t, categories, 2) + assert.Equal(t, "123", categories[0]["id"]) + assert.Equal(t, "CategoryOne", categories[0]["name"]) + assert.Equal(t, "456", categories[1]["id"]) + assert.Equal(t, "CategoryTwo", categories[1]["name"]) } From f47e5542e419e6d0bea8d3b54849ae5613344af0 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 13:35:49 +0100 Subject: [PATCH 05/26] fix out ref for linter --- pkg/github/discussions.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 3036b1796..00f6f3df5 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -75,6 +75,8 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp pagination.First = &defaultFirst } + var out []byte + var discussions []*github.Discussion if categoryID != nil { // Query with category filter (server-side filtering) @@ -124,11 +126,10 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - out, err := json.Marshal(discussions) + out, err = json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } - return mcp.NewToolResultText(string(out)), nil } else { // Query without category filter var query struct { @@ -176,12 +177,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - out, err := json.Marshal(discussions) + out, err = json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } - return mcp.NewToolResultText(string(out)), nil } + + return mcp.NewToolResultText(string(out)), nil } } From c9d6e1fbdc70d3c858b2686f794e1183d06b30c4 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 15 Jul 2025 13:44:49 +0100 Subject: [PATCH 06/26] update docs --- README.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c5274ff83..a024077ff 100644 --- a/README.md +++ b/README.md @@ -581,20 +581,28 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `repo`: Repository name (string, required) - **get_discussion_comments** - Get discussion comments + - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) + - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - `discussionNumber`: Discussion Number (number, required) + - `first`: Number of items to return per page (min 1, max 100) (number, optional) + - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - - `first`: Number of categories to return per page (min 1, max 100) (number, optional) - - `last`: Number of categories to return from the end (min 1, max 100) (number, optional) + - `first`: Number of items to return per page (min 1, max 100) (number, optional) + - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **list_discussions** - List discussions + - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) + - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) + - `first`: Number of items to return per page (min 1, max 100) (number, optional) + - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) From 9a08648eae38c70b8bf46e3cc5619a7b348f137c Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 13:02:41 +0100 Subject: [PATCH 07/26] move to unified pagination for consensus on params --- pkg/github/discussions.go | 19 +++--- pkg/github/server.go | 124 +++++++++++++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 10 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 3f69c149e..f0cf45f52 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -31,7 +31,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp mcp.WithString("category", mcp.Description("Optional filter by discussion category ID. If provided, only discussions with this category are listed."), ), - WithGraphQLPagination(), + WithUnifiedPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Required params @@ -50,11 +50,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } - // Get pagination parameters - pagination, err := OptionalGraphQLPaginationParams(request) + // Get unified pagination parameters and convert to GraphQL format + unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } + pagination := unifiedPagination.ToGraphQLParams() client, err := getGQLClient(ctx) if err != nil { @@ -272,7 +273,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati mcp.WithString("owner", mcp.Required(), mcp.Description("Repository owner")), mcp.WithString("repo", mcp.Required(), mcp.Description("Repository name")), mcp.WithNumber("discussionNumber", mcp.Required(), mcp.Description("Discussion Number")), - WithGraphQLPagination(), + WithUnifiedPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -285,10 +286,12 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati return mcp.NewToolResultError(err.Error()), nil } - paginationParams, err := OptionalGraphQLPaginationParams(request) + // Get unified pagination parameters and convert to GraphQL format + unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } + paginationParams := unifiedPagination.ToGraphQLParams() // Use default of 100 if neither first nor last is specified if paginationParams.First == nil && paginationParams.Last == nil { @@ -356,7 +359,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl mcp.Required(), mcp.Description("Repository name"), ), - WithGraphQLPagination(), + WithUnifiedPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -368,10 +371,12 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl return mcp.NewToolResultError(err.Error()), nil } - pagination, err := OptionalGraphQLPaginationParams(request) + // Get unified pagination parameters and convert to GraphQL format + unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } + pagination := unifiedPagination.ToGraphQLParams() // Use default of 100 if neither first nor last is specified if pagination.First == nil && pagination.Last == nil { diff --git a/pkg/github/server.go b/pkg/github/server.go index a537e400c..3352dcecf 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -191,10 +191,24 @@ func WithPagination() mcp.ToolOption { } } -// WithGraphQLPagination adds GraphQL cursor-based pagination parameters to a tool. -// https://docs.github.com/en/graphql/reference/objects#connection -func WithGraphQLPagination() mcp.ToolOption { +// WithUnifiedPagination adds both REST and GraphQL pagination parameters to a tool. +// This allows tools to accept both page/perPage and cursor-based pagination parameters. +// GraphQL tools should use this and convert page/perPage to first/after internally. +func WithUnifiedPagination() mcp.ToolOption { return func(tool *mcp.Tool) { + // REST API pagination parameters + mcp.WithNumber("page", + mcp.Description("Page number for pagination (min 1)"), + mcp.Min(1), + )(tool) + + mcp.WithNumber("perPage", + mcp.Description("Results per page for pagination (min 1, max 100)"), + mcp.Min(1), + mcp.Max(100), + )(tool) + + // GraphQL cursor-based pagination parameters mcp.WithNumber("first", mcp.Description("Number of items to return per page (min 1, max 100)"), mcp.Min(1), @@ -249,6 +263,110 @@ type GraphQLPaginationParams struct { Before *string } +// UnifiedPaginationParams contains both REST and GraphQL pagination parameters +type UnifiedPaginationParams struct { + // REST API pagination + Page int + PerPage int + + // GraphQL cursor-based pagination + First *int32 + Last *int32 + After *string + Before *string +} + +// ToGraphQLParams converts unified pagination parameters to GraphQL-specific parameters. +// If cursor-based parameters (first/last/after/before) are provided, they take precedence. +// Otherwise, page/perPage are converted to first/after equivalent. +func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { + // If any cursor-based parameters are explicitly set, use them directly + if u.First != nil || u.Last != nil || u.After != nil || u.Before != nil { + return GraphQLPaginationParams{ + First: u.First, + Last: u.Last, + After: u.After, + Before: u.Before, + } + } + + // Convert page/perPage to GraphQL parameters + // For GraphQL, we use 'first' for perPage and ignore page for the initial request + // (subsequent requests would use 'after' cursor from previous response) + first := int32(u.PerPage) + return GraphQLPaginationParams{ + First: &first, + Last: nil, + After: nil, + Before: nil, + } +} + +// OptionalUnifiedPaginationParams returns unified pagination parameters from the request. +// It accepts both REST API (page/perPage) and GraphQL (first/last/after/before) parameters. +func OptionalUnifiedPaginationParams(r mcp.CallToolRequest) (UnifiedPaginationParams, error) { + var params UnifiedPaginationParams + + // Get REST API pagination parameters with defaults + page, err := OptionalIntParamWithDefault(r, "page", 1) + if err != nil { + return UnifiedPaginationParams{}, err + } + params.Page = page + + perPage, err := OptionalIntParamWithDefault(r, "perPage", 30) + if err != nil { + return UnifiedPaginationParams{}, err + } + params.PerPage = perPage + + // Get GraphQL pagination parameters + if val, err := OptionalParam[float64](r, "first"); err != nil { + return UnifiedPaginationParams{}, err + } else if val != 0 { + first := int32(val) + params.First = &first + } + + if val, err := OptionalParam[float64](r, "last"); err != nil { + return UnifiedPaginationParams{}, err + } else if val != 0 { + last := int32(val) + params.Last = &last + } + + if val, err := OptionalParam[string](r, "after"); err != nil { + return UnifiedPaginationParams{}, err + } else if val != "" { + params.After = &val + } + + if val, err := OptionalParam[string](r, "before"); err != nil { + return UnifiedPaginationParams{}, err + } else if val != "" { + params.Before = &val + } + + // Validate GraphQL pagination parameters according to GraphQL connection spec + // Only validate if any GraphQL parameters are provided + if params.First != nil || params.Last != nil || params.After != nil || params.Before != nil { + if params.First != nil && params.Last != nil { + return UnifiedPaginationParams{}, fmt.Errorf("only one of 'first' or 'last' may be specified") + } + if params.After != nil && params.Before != nil { + return UnifiedPaginationParams{}, fmt.Errorf("only one of 'after' or 'before' may be specified") + } + if params.After != nil && params.Last != nil { + return UnifiedPaginationParams{}, fmt.Errorf("'after' cannot be used with 'last'. Did you mean to use 'before' instead?") + } + if params.Before != nil && params.First != nil { + return UnifiedPaginationParams{}, fmt.Errorf("'before' cannot be used with 'first'. Did you mean to use 'after' instead?") + } + } + + return params, nil +} + // OptionalGraphQLPaginationParams returns the GraphQL cursor-based pagination parameters from the request. // It validates that the parameters are used correctly according to GraphQL connection spec. func OptionalGraphQLPaginationParams(r mcp.CallToolRequest) (GraphQLPaginationParams, error) { From 5c8b4a7ad57984dfe265181700712cfb58da7a06 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 13:03:24 +0100 Subject: [PATCH 08/26] update docs --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index a024077ff..4b236d482 100644 --- a/README.md +++ b/README.md @@ -587,6 +587,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `first`: Number of items to return per page (min 1, max 100) (number, optional) - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories @@ -595,6 +597,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `first`: Number of items to return per page (min 1, max 100) (number, optional) - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussions** - List discussions @@ -604,6 +608,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `first`: Number of items to return per page (min 1, max 100) (number, optional) - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) From 6bb5daf21083de716de4c993682bf71207967e7e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 13:58:42 +0100 Subject: [PATCH 09/26] refactor pagination handling --- pkg/github/discussions.go | 57 ++++++----- pkg/github/discussions_test.go | 82 +++++++--------- pkg/github/server.go | 166 +++------------------------------ 3 files changed, 74 insertions(+), 231 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index f0cf45f52..fb04697ec 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -50,7 +50,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } - // Get unified pagination parameters and convert to GraphQL format + // Get pagination parameters and convert to GraphQL format unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -69,13 +69,6 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp categoryID = &id } - // Build GraphQL query arguments - // Use default of 30 if neither first nor last is specified - if pagination.First == nil && pagination.Last == nil { - defaultFirst := int32(30) - pagination.First = &defaultFirst - } - var out []byte var discussions []*github.Discussion @@ -97,17 +90,14 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp HasNextPage bool EndCursor string } - } `graphql:"discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId)"` + } `graphql:"discussions(first: $first, categoryId: $categoryId)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), "categoryId": *categoryID, - "first": (*githubv4.Int)(pagination.First), - "last": (*githubv4.Int)(pagination.Last), - "after": (*githubv4.String)(pagination.After), - "before": (*githubv4.String)(pagination.Before), + "first": githubv4.Int(*pagination.First), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -149,16 +139,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp HasNextPage bool EndCursor string } - } `graphql:"discussions(first: $first, last: $last, after: $after, before: $before)"` + } `graphql:"discussions(first: $first)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "first": (*githubv4.Int)(pagination.First), - "last": (*githubv4.Int)(pagination.Last), - "after": (*githubv4.String)(pagination.After), - "before": (*githubv4.String)(pagination.Before), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "first": githubv4.Int(*pagination.First), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -291,10 +278,16 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati if err != nil { return mcp.NewToolResultError(err.Error()), nil } + + // Check if pagination parameters were explicitly provided + _, pageProvided := request.Params.Arguments.(map[string]interface{})["page"] + _, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"] + paginationExplicit := pageProvided || perPageProvided + paginationParams := unifiedPagination.ToGraphQLParams() - // Use default of 100 if neither first nor last is specified - if paginationParams.First == nil && paginationParams.Last == nil { + // Use default of 100 if pagination was not explicitly provided + if !paginationExplicit { defaultFirst := int32(100) paginationParams.First = &defaultFirst } @@ -315,7 +308,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati HasNextPage githubv4.Boolean EndCursor githubv4.String } - } `graphql:"comments(first: $first, after: $after)"` + } `graphql:"comments(first: $first)"` } `graphql:"discussion(number: $discussionNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -323,8 +316,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), "discussionNumber": githubv4.Int(params.DiscussionNumber), - "first": (*githubv4.Int)(paginationParams.First), - "after": (*githubv4.String)(paginationParams.After), + "first": githubv4.Int(*paginationParams.First), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -376,10 +368,16 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl if err != nil { return mcp.NewToolResultError(err.Error()), nil } + + // Check if pagination parameters were explicitly provided + _, pageProvided := request.Params.Arguments.(map[string]interface{})["page"] + _, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"] + paginationExplicit := pageProvided || perPageProvided + pagination := unifiedPagination.ToGraphQLParams() - // Use default of 100 if neither first nor last is specified - if pagination.First == nil && pagination.Last == nil { + // Use default of 100 if pagination was not explicitly provided + if !paginationExplicit { defaultFirst := int32(100) pagination.First = &defaultFirst } @@ -400,14 +398,13 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl HasNextPage githubv4.Boolean EndCursor githubv4.String } - } `graphql:"discussionCategories(first: $first, after: $after)"` + } `graphql:"discussionCategories(first: $first)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), - "first": (*githubv4.Int)(pagination.First), - "after": (*githubv4.String)(pagination.After), + "first": githubv4.Int(*pagination.First), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index fb63fa7a8..771578825 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -61,37 +61,28 @@ func Test_ListDiscussions(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) // Use exact string queries that match implementation output (from error messages) - qDiscussions := "query($after:String$before:String$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussions := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" - qDiscussionsFiltered := "query($after:String$before:String$categoryId:ID!$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussionsFiltered := "query($categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling varsListAll := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "first": float64(30), - "last": nil, - "after": nil, - "before": nil, + "owner": "owner", + "repo": "repo", + "first": float64(30), } varsRepoNotFound := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("nonexistent-repo"), - "first": float64(30), - "last": nil, - "after": nil, - "before": nil, + "owner": "owner", + "repo": "nonexistent-repo", + "first": float64(30), } varsDiscussionsFiltered := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "categoryId": githubv4.ID("DIC_kwDOABC123"), + "owner": "owner", + "repo": "repo", + "categoryId": "DIC_kwDOABC123", "first": float64(30), - "last": nil, - "after": nil, - "before": nil, } tests := []struct { @@ -191,23 +182,13 @@ func Test_GetDiscussion(t *testing.T) { assert.Contains(t, toolDef.InputSchema.Properties, "discussionNumber") assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) - var q struct { - Repository struct { - Discussion struct { - Number githubv4.Int - Body githubv4.String - CreatedAt githubv4.DateTime - URL githubv4.String `graphql:"url"` - Category struct { - Name githubv4.String - } `graphql:"category"` - } `graphql:"discussion(number: $discussionNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + // Use exact string query that matches implementation output + qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,body,createdAt,url,category{name}}}}" + vars := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "discussionNumber": githubv4.Int(1), + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), } tests := []struct { name string @@ -247,7 +228,7 @@ func Test_GetDiscussion(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - matcher := githubv4mock.NewQueryMatcher(q, vars, tc.response) + matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, tc.response) httpClient := githubv4mock.NewMockedHTTPClient(matcher) gqlClient := githubv4.NewClient(httpClient) _, handler := GetDiscussion(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) @@ -285,15 +266,14 @@ func Test_GetDiscussionComments(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) // Use exact string query that matches implementation output - 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,endCursor}}}}}" + qGetComments := "query($discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first){nodes{body},pageInfo{hasNextPage,endCursor}}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "discussionNumber": githubv4.Int(1), - "first": float64(100), // Default value when no pagination specified - "after": nil, + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + "first": float64(100), } mockResponse := githubv4mock.DataResponse(map[string]any{ @@ -328,6 +308,9 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) + // Debug: print the actual JSON response + t.Logf("JSON response: %s", textContent.Text) + var comments []*github.IssueComment err = json.Unmarshal([]byte(textContent.Text), &comments) require.NoError(t, err) @@ -340,14 +323,13 @@ func Test_GetDiscussionComments(t *testing.T) { func Test_ListDiscussionCategories(t *testing.T) { // Use exact string query that matches implementation output - qListCategories := "query($after:String$first:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" + qListCategories := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "first": float64(100), // Default value when no pagination specified - "after": nil, + "owner": "owner", + "repo": "repo", + "first": float64(100), } mockResp := githubv4mock.DataResponse(map[string]any{ @@ -380,6 +362,10 @@ func Test_ListDiscussionCategories(t *testing.T) { require.NoError(t, err) text := getTextResult(t, result).Text + + // Debug: print the actual JSON response + t.Logf("JSON response: %s", text) + var categories []map[string]string require.NoError(t, json.Unmarshal([]byte(text), &categories)) assert.Len(t, categories, 2) diff --git a/pkg/github/server.go b/pkg/github/server.go index 3352dcecf..f8632c2de 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -191,12 +191,10 @@ func WithPagination() mcp.ToolOption { } } -// WithUnifiedPagination adds both REST and GraphQL pagination parameters to a tool. -// This allows tools to accept both page/perPage and cursor-based pagination parameters. -// GraphQL tools should use this and convert page/perPage to first/after internally. +// WithUnifiedPagination adds REST API pagination parameters to a tool. +// GraphQL tools will use this and convert page/perPage to GraphQL cursor parameters internally. func WithUnifiedPagination() mcp.ToolOption { return func(tool *mcp.Tool) { - // REST API pagination parameters mcp.WithNumber("page", mcp.Description("Page number for pagination (min 1)"), mcp.Min(1), @@ -207,27 +205,6 @@ func WithUnifiedPagination() mcp.ToolOption { mcp.Min(1), mcp.Max(100), )(tool) - - // GraphQL cursor-based pagination parameters - mcp.WithNumber("first", - mcp.Description("Number of items to return per page (min 1, max 100)"), - mcp.Min(1), - mcp.Max(100), - )(tool) - - mcp.WithNumber("last", - mcp.Description("Number of items to return from the end (min 1, max 100)"), - mcp.Min(1), - mcp.Max(100), - )(tool) - - mcp.WithString("after", - mcp.Description("Cursor for pagination, use the 'after' field from the previous response"), - )(tool) - - mcp.WithString("before", - mcp.Description("Cursor for pagination, use the 'before' field from the previous response"), - )(tool) } } @@ -257,162 +234,45 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) { } type GraphQLPaginationParams struct { - First *int32 - Last *int32 - After *string - Before *string + First *int32 } -// UnifiedPaginationParams contains both REST and GraphQL pagination parameters +// UnifiedPaginationParams contains REST API pagination parameters that can be converted to GraphQL internally type UnifiedPaginationParams struct { - // REST API pagination Page int PerPage int - - // GraphQL cursor-based pagination - First *int32 - Last *int32 - After *string - Before *string } -// ToGraphQLParams converts unified pagination parameters to GraphQL-specific parameters. -// If cursor-based parameters (first/last/after/before) are provided, they take precedence. -// Otherwise, page/perPage are converted to first/after equivalent. +// ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. +// This converts page/perPage to first parameter for GraphQL queries. func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { - // If any cursor-based parameters are explicitly set, use them directly - if u.First != nil || u.Last != nil || u.After != nil || u.Before != nil { - return GraphQLPaginationParams{ - First: u.First, - Last: u.Last, - After: u.After, - Before: u.Before, - } - } - // Convert page/perPage to GraphQL parameters // For GraphQL, we use 'first' for perPage and ignore page for the initial request // (subsequent requests would use 'after' cursor from previous response) first := int32(u.PerPage) return GraphQLPaginationParams{ - First: &first, - Last: nil, - After: nil, - Before: nil, + First: &first, } } -// OptionalUnifiedPaginationParams returns unified pagination parameters from the request. -// It accepts both REST API (page/perPage) and GraphQL (first/last/after/before) parameters. +// OptionalUnifiedPaginationParams returns pagination parameters from the request. +// It accepts REST API (page/perPage) parameters only. func OptionalUnifiedPaginationParams(r mcp.CallToolRequest) (UnifiedPaginationParams, error) { - var params UnifiedPaginationParams - // Get REST API pagination parameters with defaults page, err := OptionalIntParamWithDefault(r, "page", 1) if err != nil { return UnifiedPaginationParams{}, err } - params.Page = page perPage, err := OptionalIntParamWithDefault(r, "perPage", 30) if err != nil { return UnifiedPaginationParams{}, err } - params.PerPage = perPage - - // Get GraphQL pagination parameters - if val, err := OptionalParam[float64](r, "first"); err != nil { - return UnifiedPaginationParams{}, err - } else if val != 0 { - first := int32(val) - params.First = &first - } - - if val, err := OptionalParam[float64](r, "last"); err != nil { - return UnifiedPaginationParams{}, err - } else if val != 0 { - last := int32(val) - params.Last = &last - } - - if val, err := OptionalParam[string](r, "after"); err != nil { - return UnifiedPaginationParams{}, err - } else if val != "" { - params.After = &val - } - if val, err := OptionalParam[string](r, "before"); err != nil { - return UnifiedPaginationParams{}, err - } else if val != "" { - params.Before = &val - } - - // Validate GraphQL pagination parameters according to GraphQL connection spec - // Only validate if any GraphQL parameters are provided - if params.First != nil || params.Last != nil || params.After != nil || params.Before != nil { - if params.First != nil && params.Last != nil { - return UnifiedPaginationParams{}, fmt.Errorf("only one of 'first' or 'last' may be specified") - } - if params.After != nil && params.Before != nil { - return UnifiedPaginationParams{}, fmt.Errorf("only one of 'after' or 'before' may be specified") - } - if params.After != nil && params.Last != nil { - return UnifiedPaginationParams{}, fmt.Errorf("'after' cannot be used with 'last'. Did you mean to use 'before' instead?") - } - if params.Before != nil && params.First != nil { - return UnifiedPaginationParams{}, fmt.Errorf("'before' cannot be used with 'first'. Did you mean to use 'after' instead?") - } - } - - return params, nil -} - -// OptionalGraphQLPaginationParams returns the GraphQL cursor-based pagination parameters from the request. -// It validates that the parameters are used correctly according to GraphQL connection spec. -func OptionalGraphQLPaginationParams(r mcp.CallToolRequest) (GraphQLPaginationParams, error) { - var params GraphQLPaginationParams - - if val, err := OptionalParam[float64](r, "first"); err != nil { - return GraphQLPaginationParams{}, err - } else if val != 0 { - first := int32(val) - params.First = &first - } - - if val, err := OptionalParam[float64](r, "last"); err != nil { - return GraphQLPaginationParams{}, err - } else if val != 0 { - last := int32(val) - params.Last = &last - } - - if val, err := OptionalParam[string](r, "after"); err != nil { - return GraphQLPaginationParams{}, err - } else if val != "" { - params.After = &val - } - - if val, err := OptionalParam[string](r, "before"); err != nil { - return GraphQLPaginationParams{}, err - } else if val != "" { - params.Before = &val - } - - // Validate pagination parameters according to GraphQL connection spec - if params.First != nil && params.Last != nil { - return GraphQLPaginationParams{}, fmt.Errorf("only one of 'first' or 'last' may be specified") - } - if params.After != nil && params.Before != nil { - return GraphQLPaginationParams{}, fmt.Errorf("only one of 'after' or 'before' may be specified") - } - if params.After != nil && params.Last != nil { - return GraphQLPaginationParams{}, fmt.Errorf("'after' cannot be used with 'last'. Did you mean to use 'before' instead?") - } - if params.Before != nil && params.First != nil { - return GraphQLPaginationParams{}, fmt.Errorf("'before' cannot be used with 'first'. Did you mean to use 'after' instead?") - } - - return params, nil + return UnifiedPaginationParams{ + Page: page, + PerPage: perPage, + }, nil } func MarshalledTextResult(v any) *mcp.CallToolResult { From 6d00394f75f2d3a7f1ed84818d6e5f29e7ebcff0 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 14:00:02 +0100 Subject: [PATCH 10/26] update docs --- README.md | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/README.md b/README.md index 4b236d482..5fe75e0ea 100644 --- a/README.md +++ b/README.md @@ -581,32 +581,20 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `repo`: Repository name (string, required) - **get_discussion_comments** - Get discussion comments - - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) - - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - `discussionNumber`: Discussion Number (number, required) - - `first`: Number of items to return per page (min 1, max 100) (number, optional) - - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories - - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) - - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - - `first`: Number of items to return per page (min 1, max 100) (number, optional) - - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussions** - List discussions - - `after`: Cursor for pagination, use the 'after' field from the previous response (string, optional) - - `before`: Cursor for pagination, use the 'before' field from the previous response (string, optional) - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) - - `first`: Number of items to return per page (min 1, max 100) (number, optional) - - `last`: Number of items to return from the end (min 1, max 100) (number, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) From d2dd8f2e0522d647c8b0f014cd19f9c270147ad7 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:33:49 +0100 Subject: [PATCH 11/26] linter fix --- pkg/github/server.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index f8632c2de..8114273ea 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -246,10 +246,12 @@ type UnifiedPaginationParams struct { // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { - // Convert page/perPage to GraphQL parameters - // For GraphQL, we use 'first' for perPage and ignore page for the initial request - // (subsequent requests would use 'after' cursor from previous response) - first := int32(u.PerPage) + perPage := u.PerPage + if perPage > 2147483647 { + perPage = 100 + } + first := int32(perPage) + return GraphQLPaginationParams{ First: &first, } From 3cab7b11506740fed199c7365499c7c3cb9fb94d Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:36:18 +0100 Subject: [PATCH 12/26] conv rest to gql params for safe lint --- pkg/github/server.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index 8114273ea..80b4f5069 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -246,10 +246,14 @@ type UnifiedPaginationParams struct { // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { + // Cap perPage to safe range before converting to int32 perPage := u.PerPage - if perPage > 2147483647 { + if perPage > 100 { // GraphQL expects a max of 100 perPage = 100 } + if perPage < 1 { + perPage = 1 + } first := int32(perPage) return GraphQLPaginationParams{ From f701670c9a9fb67b323422f128e8dc4c5c46ddbc Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:38:44 +0100 Subject: [PATCH 13/26] add nolint --- pkg/github/server.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index 80b4f5069..b9856b4ba 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -246,18 +246,9 @@ type UnifiedPaginationParams struct { // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { - // Cap perPage to safe range before converting to int32 - perPage := u.PerPage - if perPage > 100 { // GraphQL expects a max of 100 - perPage = 100 - } - if perPage < 1 { - perPage = 1 - } - first := int32(perPage) - + first := int32(u.PerPage) return GraphQLPaginationParams{ - First: &first, + First: &first, //nolint:gosec // G115: This is safe, we cap perPage to 100 in the toolset } } From 30e01f4607e8c1b6a3bfbdb3a4e9ce8828f8050e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:47:20 +0100 Subject: [PATCH 14/26] add error handling for perPage value in ToGraphQLParams --- pkg/github/server.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index b9856b4ba..74453be5f 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -245,11 +245,14 @@ type UnifiedPaginationParams struct { // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. -func (u UnifiedPaginationParams) ToGraphQLParams() GraphQLPaginationParams { +func (u UnifiedPaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { + if u.PerPage > 100 { + return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d exceeds maximum of 100", u.PerPage) + } first := int32(u.PerPage) return GraphQLPaginationParams{ - First: &first, //nolint:gosec // G115: This is safe, we cap perPage to 100 in the toolset - } + First: &first, + }, nil } // OptionalUnifiedPaginationParams returns pagination parameters from the request. From b4549269e23ce9c827662978c9a8f49112c59e6e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:48:09 +0100 Subject: [PATCH 15/26] refactor pagination error handling --- pkg/github/discussions.go | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index fb04697ec..5a51e4e21 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -53,9 +53,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp // Get pagination parameters and convert to GraphQL format unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { - return mcp.NewToolResultError(err.Error()), nil + return nil, err + } + pagination, err := unifiedPagination.ToGraphQLParams() + if err != nil { + return nil, err } - pagination := unifiedPagination.ToGraphQLParams() client, err := getGQLClient(ctx) if err != nil { @@ -276,15 +279,18 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati // Get unified pagination parameters and convert to GraphQL format unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { - return mcp.NewToolResultError(err.Error()), nil + return nil, err } // Check if pagination parameters were explicitly provided - _, pageProvided := request.Params.Arguments.(map[string]interface{})["page"] - _, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"] + _, pageProvided := request.GetArguments()["page"] + _, perPageProvided := request.GetArguments()["perPage"] paginationExplicit := pageProvided || perPageProvided - paginationParams := unifiedPagination.ToGraphQLParams() + paginationParams, err := unifiedPagination.ToGraphQLParams() + if err != nil { + return nil, err + } // Use default of 100 if pagination was not explicitly provided if !paginationExplicit { @@ -366,15 +372,18 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl // Get unified pagination parameters and convert to GraphQL format unifiedPagination, err := OptionalUnifiedPaginationParams(request) if err != nil { - return mcp.NewToolResultError(err.Error()), nil + return nil, err } // Check if pagination parameters were explicitly provided - _, pageProvided := request.Params.Arguments.(map[string]interface{})["page"] - _, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"] + _, pageProvided := request.GetArguments()["page"] + _, perPageProvided := request.GetArguments()["perPage"] paginationExplicit := pageProvided || perPageProvided - pagination := unifiedPagination.ToGraphQLParams() + pagination, err := unifiedPagination.ToGraphQLParams() + if err != nil { + return nil, err + } // Use default of 100 if pagination was not explicitly provided if !paginationExplicit { From 98fd144d86cf1fe60a333899b5bd10f6df221ba5 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 15:56:43 +0100 Subject: [PATCH 16/26] unified params for rest andn graphql and rennamed to be uniform for golang --- pkg/github/actions.go | 16 ++++++------- pkg/github/discussions.go | 24 ++++++++++---------- pkg/github/issues.go | 4 ++-- pkg/github/notifications.go | 4 ++-- pkg/github/pullrequests.go | 8 +++---- pkg/github/repositories.go | 16 ++++++------- pkg/github/search.go | 12 +++++----- pkg/github/search_utils.go | 4 ++-- pkg/github/server.go | 45 +++++++++---------------------------- pkg/github/server_test.go | 16 ++++++------- 10 files changed, 63 insertions(+), 86 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 3c441d5aa..19b56389c 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -62,8 +62,8 @@ func ListWorkflows(getClient GetClientFn, t translations.TranslationHelperFunc) // Set up list options opts := &github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, } workflows, resp, err := client.Actions.ListWorkflows(ctx, owner, repo, opts) @@ -200,8 +200,8 @@ func ListWorkflowRuns(getClient GetClientFn, t translations.TranslationHelperFun Event: event, Status: status, ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, }, } @@ -503,8 +503,8 @@ func ListWorkflowJobs(getClient GetClientFn, t translations.TranslationHelperFun opts := &github.ListWorkflowJobsOptions{ Filter: filter, ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, }, } @@ -1025,8 +1025,8 @@ func ListWorkflowRunArtifacts(getClient GetClientFn, t translations.TranslationH // Set up list options opts := &github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, } artifacts, resp, err := client.Actions.ListWorkflowRunArtifacts(ctx, owner, repo, runID, opts) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 5a51e4e21..4a80216b0 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -51,11 +51,11 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } // Get pagination parameters and convert to GraphQL format - unifiedPagination, err := OptionalUnifiedPaginationParams(request) + pagination, err := OptionalPaginationParams(request) if err != nil { return nil, err } - pagination, err := unifiedPagination.ToGraphQLParams() + paginationParams, err := pagination.ToGraphQLParams() if err != nil { return nil, err } @@ -100,7 +100,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp "owner": githubv4.String(owner), "repo": githubv4.String(repo), "categoryId": *categoryID, - "first": githubv4.Int(*pagination.First), + "first": githubv4.Int(*paginationParams.First), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -148,7 +148,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp vars := map[string]interface{}{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), - "first": githubv4.Int(*pagination.First), + "first": githubv4.Int(*paginationParams.First), } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -276,8 +276,8 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati return mcp.NewToolResultError(err.Error()), nil } - // Get unified pagination parameters and convert to GraphQL format - unifiedPagination, err := OptionalUnifiedPaginationParams(request) + // Get pagination parameters and convert to GraphQL format + pagination, err := OptionalPaginationParams(request) if err != nil { return nil, err } @@ -287,7 +287,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati _, perPageProvided := request.GetArguments()["perPage"] paginationExplicit := pageProvided || perPageProvided - paginationParams, err := unifiedPagination.ToGraphQLParams() + paginationParams, err := pagination.ToGraphQLParams() if err != nil { return nil, err } @@ -369,8 +369,8 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl return mcp.NewToolResultError(err.Error()), nil } - // Get unified pagination parameters and convert to GraphQL format - unifiedPagination, err := OptionalUnifiedPaginationParams(request) + // Get pagination parameters and convert to GraphQL format + pagination, err := OptionalPaginationParams(request) if err != nil { return nil, err } @@ -380,7 +380,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl _, perPageProvided := request.GetArguments()["perPage"] paginationExplicit := pageProvided || perPageProvided - pagination, err := unifiedPagination.ToGraphQLParams() + paginationParams, err := pagination.ToGraphQLParams() if err != nil { return nil, err } @@ -388,7 +388,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl // Use default of 100 if pagination was not explicitly provided if !paginationExplicit { defaultFirst := int32(100) - pagination.First = &defaultFirst + paginationParams.First = &defaultFirst } client, err := getGQLClient(ctx) @@ -413,7 +413,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl vars := map[string]interface{}{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), - "first": githubv4.Int(*pagination.First), + "first": githubv4.Int(*paginationParams.First), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 29d32bd18..80a150aba 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -630,8 +630,8 @@ func GetIssueComments(getClient GetClientFn, t translations.TranslationHelperFun opts := &github.IssueListCommentsOptions{ ListOptions: github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, }, } diff --git a/pkg/github/notifications.go b/pkg/github/notifications.go index a41edaf42..fdd418098 100644 --- a/pkg/github/notifications.go +++ b/pkg/github/notifications.go @@ -88,8 +88,8 @@ func ListNotifications(getClient GetClientFn, t translations.TranslationHelperFu All: filter == FilterIncludeRead, Participating: filter == FilterOnlyParticipating, ListOptions: github.ListOptions{ - Page: paginationParams.page, - PerPage: paginationParams.perPage, + Page: paginationParams.Page, + PerPage: paginationParams.PerPage, }, } diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index aeca650fa..aea91a29f 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -403,8 +403,8 @@ func ListPullRequests(getClient GetClientFn, t translations.TranslationHelperFun Sort: sort, Direction: direction, ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, }, } @@ -622,8 +622,8 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper return nil, fmt.Errorf("failed to get GitHub client: %w", err) } opts := &github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, } files, resp, err := client.PullRequests.ListFiles(ctx, owner, repo, pullNumber, opts) if err != nil { diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 58e4a7421..cbf77bf39 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -58,8 +58,8 @@ func GetCommit(getClient GetClientFn, t translations.TranslationHelperFunc) (too } opts := &github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, } client, err := getClient(ctx) @@ -139,7 +139,7 @@ func ListCommits(getClient GetClientFn, t translations.TranslationHelperFunc) (t return mcp.NewToolResultError(err.Error()), nil } // Set default perPage to 30 if not provided - perPage := pagination.perPage + perPage := pagination.PerPage if perPage == 0 { perPage = 30 } @@ -147,7 +147,7 @@ func ListCommits(getClient GetClientFn, t translations.TranslationHelperFunc) (t SHA: sha, Author: author, ListOptions: github.ListOptions{ - Page: pagination.page, + Page: pagination.Page, PerPage: perPage, }, } @@ -217,8 +217,8 @@ func ListBranches(getClient GetClientFn, t translations.TranslationHelperFunc) ( opts := &github.BranchListOptions{ ListOptions: github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, }, } @@ -1170,8 +1170,8 @@ func ListTags(getClient GetClientFn, t translations.TranslationHelperFunc) (tool } opts := &github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, } client, err := getClient(ctx) diff --git a/pkg/github/search.go b/pkg/github/search.go index 04a1facc0..b11bb3bbc 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -39,8 +39,8 @@ func SearchRepositories(getClient GetClientFn, t translations.TranslationHelperF opts := &github.SearchOptions{ ListOptions: github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, }, } @@ -118,8 +118,8 @@ func SearchCode(getClient GetClientFn, t translations.TranslationHelperFunc) (to Sort: sort, Order: order, ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, }, } @@ -193,8 +193,8 @@ func userOrOrgHandler(accountType string, getClient GetClientFn) server.ToolHand Sort: sort, Order: order, ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, + PerPage: pagination.PerPage, + Page: pagination.Page, }, } diff --git a/pkg/github/search_utils.go b/pkg/github/search_utils.go index 5dd48040e..a6ff1f782 100644 --- a/pkg/github/search_utils.go +++ b/pkg/github/search_utils.go @@ -56,8 +56,8 @@ func searchHandler( Sort: sort, Order: order, ListOptions: github.ListOptions{ - Page: pagination.page, - PerPage: pagination.perPage, + Page: pagination.Page, + PerPage: pagination.PerPage, }, } diff --git a/pkg/github/server.go b/pkg/github/server.go index 74453be5f..ceb0b0b96 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -209,8 +209,8 @@ func WithUnifiedPagination() mcp.ToolOption { } type PaginationParams struct { - page int - perPage int + Page int + PerPage int } // OptionalPaginationParams returns the "page" and "perPage" parameters from the request, @@ -228,8 +228,8 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) { return PaginationParams{}, err } return PaginationParams{ - page: page, - perPage: perPage, + Page: page, + PerPage: perPage, }, nil } @@ -237,44 +237,21 @@ type GraphQLPaginationParams struct { First *int32 } -// UnifiedPaginationParams contains REST API pagination parameters that can be converted to GraphQL internally -type UnifiedPaginationParams struct { - Page int - PerPage int -} - // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. -func (u UnifiedPaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { - if u.PerPage > 100 { - return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d exceeds maximum of 100", u.PerPage) +func (p PaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { + if p.PerPage > 100 { + return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d exceeds maximum of 100", p.PerPage) + } + if p.PerPage < 0 { + return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d cannot be negative", p.PerPage) } - first := int32(u.PerPage) + first := int32(p.PerPage) return GraphQLPaginationParams{ First: &first, }, nil } -// OptionalUnifiedPaginationParams returns pagination parameters from the request. -// It accepts REST API (page/perPage) parameters only. -func OptionalUnifiedPaginationParams(r mcp.CallToolRequest) (UnifiedPaginationParams, error) { - // Get REST API pagination parameters with defaults - page, err := OptionalIntParamWithDefault(r, "page", 1) - if err != nil { - return UnifiedPaginationParams{}, err - } - - perPage, err := OptionalIntParamWithDefault(r, "perPage", 30) - if err != nil { - return UnifiedPaginationParams{}, err - } - - return UnifiedPaginationParams{ - Page: page, - PerPage: perPage, - }, nil -} - func MarshalledTextResult(v any) *mcp.CallToolResult { data, err := json.Marshal(v) if err != nil { diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 6353f254d..7f8f29c0d 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -489,8 +489,8 @@ func TestOptionalPaginationParams(t *testing.T) { name: "no pagination parameters, default values", params: map[string]any{}, expected: PaginationParams{ - page: 1, - perPage: 30, + Page: 1, + PerPage: 30, }, expectError: false, }, @@ -500,8 +500,8 @@ func TestOptionalPaginationParams(t *testing.T) { "page": float64(2), }, expected: PaginationParams{ - page: 2, - perPage: 30, + Page: 2, + PerPage: 30, }, expectError: false, }, @@ -511,8 +511,8 @@ func TestOptionalPaginationParams(t *testing.T) { "perPage": float64(50), }, expected: PaginationParams{ - page: 1, - perPage: 50, + Page: 1, + PerPage: 50, }, expectError: false, }, @@ -523,8 +523,8 @@ func TestOptionalPaginationParams(t *testing.T) { "perPage": float64(50), }, expected: PaginationParams{ - page: 2, - perPage: 50, + Page: 2, + PerPage: 50, }, expectError: false, }, From 109999086834da05fca93e22d6c066fa1dc59647 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 16:20:46 +0100 Subject: [PATCH 17/26] add 'after' for pagination --- pkg/github/discussions.go | 72 ++++++++++++++++++++++++++++++---- pkg/github/discussions_test.go | 61 +++++++++++++++++++--------- pkg/github/server.go | 21 +++++++++- 3 files changed, 126 insertions(+), 28 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 4a80216b0..5ba40a492 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -93,7 +93,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp HasNextPage bool EndCursor string } - } `graphql:"discussions(first: $first, categoryId: $categoryId)"` + } `graphql:"discussions(first: $first, after: $after, categoryId: $categoryId)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ @@ -102,6 +102,11 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp "categoryId": *categoryID, "first": githubv4.Int(*paginationParams.First), } + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + vars["after"] = (*githubv4.String)(nil) + } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -120,7 +125,16 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - out, err = json.Marshal(discussions) + // Create response with pagination info + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + }, + } + + out, err = json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } @@ -142,7 +156,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp HasNextPage bool EndCursor string } - } `graphql:"discussions(first: $first)"` + } `graphql:"discussions(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ @@ -150,6 +164,11 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp "repo": githubv4.String(repo), "first": githubv4.Int(*paginationParams.First), } + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + vars["after"] = (*githubv4.String)(nil) + } if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -168,7 +187,16 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, di) } - out, err = json.Marshal(discussions) + // Create response with pagination info + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + }, + } + + out, err = json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) } @@ -314,7 +342,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati HasNextPage githubv4.Boolean EndCursor githubv4.String } - } `graphql:"comments(first: $first)"` + } `graphql:"comments(first: $first, after: $after)"` } `graphql:"discussion(number: $discussionNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -324,6 +352,11 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati "discussionNumber": githubv4.Int(params.DiscussionNumber), "first": githubv4.Int(*paginationParams.First), } + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + vars["after"] = (*githubv4.String)(nil) + } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -333,7 +366,16 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati comments = append(comments, &github.IssueComment{Body: github.Ptr(string(c.Body))}) } - out, err := json.Marshal(comments) + // Create response with pagination info + response := map[string]interface{}{ + "comments": comments, + "pageInfo": map[string]interface{}{ + "hasNextPage": q.Repository.Discussion.Comments.PageInfo.HasNextPage, + "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), + }, + } + + out, err := json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal comments: %w", err) } @@ -407,7 +449,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl HasNextPage githubv4.Boolean EndCursor githubv4.String } - } `graphql:"discussionCategories(first: $first)"` + } `graphql:"discussionCategories(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ @@ -415,6 +457,11 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl "repo": githubv4.String(params.Repo), "first": githubv4.Int(*paginationParams.First), } + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + vars["after"] = (*githubv4.String)(nil) + } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -427,7 +474,16 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl }) } - out, err := json.Marshal(categories) + // Create response with pagination info + response := map[string]interface{}{ + "categories": categories, + "pageInfo": map[string]interface{}{ + "hasNextPage": q.Repository.DiscussionCategories.PageInfo.HasNextPage, + "endCursor": string(q.Repository.DiscussionCategories.PageInfo.EndCursor), + }, + } + + out, err := json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal discussion categories: %w", err) } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 771578825..3504bb92b 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -61,21 +61,23 @@ func Test_ListDiscussions(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) // Use exact string queries that match implementation output (from error messages) - qDiscussions := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussions := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" - qDiscussionsFiltered := "query($categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussionsFiltered := "query($after:String$categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling varsListAll := map[string]interface{}{ "owner": "owner", "repo": "repo", "first": float64(30), + "after": (*string)(nil), } varsRepoNotFound := map[string]interface{}{ "owner": "owner", "repo": "nonexistent-repo", "first": float64(30), + "after": (*string)(nil), } varsDiscussionsFiltered := map[string]interface{}{ @@ -83,6 +85,7 @@ func Test_ListDiscussions(t *testing.T) { "repo": "repo", "categoryId": "DIC_kwDOABC123", "first": float64(30), + "after": (*string)(nil), } tests := []struct { @@ -155,15 +158,21 @@ func Test_ListDiscussions(t *testing.T) { require.NoError(t, err) // Parse the structured response with pagination info - var returnedDiscussions []*github.Discussion - err = json.Unmarshal([]byte(text), &returnedDiscussions) + var response struct { + Discussions []*github.Discussion `json:"discussions"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + } + err = json.Unmarshal([]byte(text), &response) require.NoError(t, err) - assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions)) + assert.Len(t, response.Discussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(response.Discussions)) // Verify that all returned discussions have a category if filtered if _, hasCategory := tc.reqParams["category"]; hasCategory { - for _, discussion := range returnedDiscussions { + for _, discussion := range response.Discussions { require.NotNil(t, discussion.DiscussionCategory, "Discussion should have category") assert.NotEmpty(t, *discussion.DiscussionCategory.Name, "Discussion should have category name") } @@ -266,7 +275,7 @@ func Test_GetDiscussionComments(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) // Use exact string query that matches implementation output - qGetComments := "query($discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first){nodes{body},pageInfo{hasNextPage,endCursor}}}}}" + 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,endCursor}}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ @@ -274,6 +283,7 @@ func Test_GetDiscussionComments(t *testing.T) { "repo": "repo", "discussionNumber": float64(1), "first": float64(100), + "after": (*string)(nil), } mockResponse := githubv4mock.DataResponse(map[string]any{ @@ -311,25 +321,32 @@ func Test_GetDiscussionComments(t *testing.T) { // Debug: print the actual JSON response t.Logf("JSON response: %s", textContent.Text) - var comments []*github.IssueComment - err = json.Unmarshal([]byte(textContent.Text), &comments) + var response struct { + Comments []*github.IssueComment `json:"comments"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + } + err = json.Unmarshal([]byte(textContent.Text), &response) require.NoError(t, err) - assert.Len(t, comments, 2) + assert.Len(t, response.Comments, 2) expectedBodies := []string{"This is the first comment", "This is the second comment"} - for i, comment := range comments { + for i, comment := range response.Comments { assert.Equal(t, expectedBodies[i], *comment.Body) } } func Test_ListDiscussionCategories(t *testing.T) { // Use exact string query that matches implementation output - qListCategories := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" + qListCategories := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ "owner": "owner", "repo": "repo", "first": float64(100), + "after": (*string)(nil), } mockResp := githubv4mock.DataResponse(map[string]any{ @@ -366,11 +383,17 @@ func Test_ListDiscussionCategories(t *testing.T) { // Debug: print the actual JSON response t.Logf("JSON response: %s", text) - var categories []map[string]string - require.NoError(t, json.Unmarshal([]byte(text), &categories)) - assert.Len(t, categories, 2) - assert.Equal(t, "123", categories[0]["id"]) - assert.Equal(t, "CategoryOne", categories[0]["name"]) - assert.Equal(t, "456", categories[1]["id"]) - assert.Equal(t, "CategoryTwo", categories[1]["name"]) + var response struct { + Categories []map[string]string `json:"categories"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + } + require.NoError(t, json.Unmarshal([]byte(text), &response)) + assert.Len(t, response.Categories, 2) + assert.Equal(t, "123", response.Categories[0]["id"]) + assert.Equal(t, "CategoryOne", response.Categories[0]["name"]) + assert.Equal(t, "456", response.Categories[1]["id"]) + assert.Equal(t, "CategoryTwo", response.Categories[1]["name"]) } diff --git a/pkg/github/server.go b/pkg/github/server.go index ceb0b0b96..9b109c1f0 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -205,15 +205,20 @@ func WithUnifiedPagination() mcp.ToolOption { mcp.Min(1), mcp.Max(100), )(tool) + + mcp.WithString("after", + mcp.Description("Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs."), + )(tool) } } type PaginationParams struct { Page int PerPage int + After string } -// OptionalPaginationParams returns the "page" and "perPage" parameters from the request, +// OptionalPaginationParams returns the "page", "perPage", and "after" parameters from the request, // or their default values if not present, "page" default is 1, "perPage" default is 30. // In future, we may want to make the default values configurable, or even have this // function returned from `withPagination`, where the defaults are provided alongside @@ -227,18 +232,25 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) { if err != nil { return PaginationParams{}, err } + after, err := OptionalParam[string](r, "after") + if err != nil { + return PaginationParams{}, err + } return PaginationParams{ Page: page, PerPage: perPage, + After: after, }, nil } type GraphQLPaginationParams struct { First *int32 + After *string } // ToGraphQLParams converts REST API pagination parameters to GraphQL-specific parameters. // This converts page/perPage to first parameter for GraphQL queries. +// If After is provided, it takes precedence over page-based pagination. func (p PaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { if p.PerPage > 100 { return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d exceeds maximum of 100", p.PerPage) @@ -247,8 +259,15 @@ func (p PaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d cannot be negative", p.PerPage) } first := int32(p.PerPage) + + var after *string + if p.After != "" { + after = &p.After + } + return GraphQLPaginationParams{ First: &first, + After: after, }, nil } From 752499bc4072e075798e09160f5a0ec8ceb75c8b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 16 Jul 2025 16:27:37 +0100 Subject: [PATCH 18/26] update docs --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 5fe75e0ea..6757e9bfa 100644 --- a/README.md +++ b/README.md @@ -581,6 +581,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `repo`: Repository name (string, required) - **get_discussion_comments** - Get discussion comments + - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `discussionNumber`: Discussion Number (number, required) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) @@ -588,12 +589,14 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories + - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussions** - List discussions + - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) From 548d4d7f91712c66c976d28478b7272237cc8c17 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 15:43:07 +0100 Subject: [PATCH 19/26] Update pkg/github/discussions.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/discussions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 5ba40a492..c70f6eaf7 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -322,7 +322,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati // Use default of 100 if pagination was not explicitly provided if !paginationExplicit { - defaultFirst := int32(100) + defaultFirst := int32(DefaultGraphQLPageSize) paginationParams.First = &defaultFirst } From 592940ef8600a29ccf1c3b81fd361224b8b38564 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 15:43:20 +0100 Subject: [PATCH 20/26] Update pkg/github/discussions.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/discussions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index c70f6eaf7..7c7a13740 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -429,7 +429,7 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl // Use default of 100 if pagination was not explicitly provided if !paginationExplicit { - defaultFirst := int32(100) + defaultFirst := int32(DefaultGraphQLPageSize) paginationParams.First = &defaultFirst } From b09612e5a7972ad3e8303b61d26d6658ed4f5532 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 15:43:29 +0100 Subject: [PATCH 21/26] Update pkg/github/discussions_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/discussions_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 3504bb92b..70e997ec3 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -318,8 +318,7 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) - // Debug: print the actual JSON response - t.Logf("JSON response: %s", textContent.Text) +// (Lines removed) var response struct { Comments []*github.IssueComment `json:"comments"` From e21635c4183e3b216d7f78b5d94af75fd2006669 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 15:44:58 +0100 Subject: [PATCH 22/26] update default page size const --- pkg/github/discussions.go | 2 ++ pkg/github/discussions_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 7c7a13740..4e6837837 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -13,6 +13,8 @@ import ( "github.com/shurcooL/githubv4" ) +const DefaultGraphQLPageSize = 30 + func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("list_discussions", mcp.WithDescription(t("TOOL_LIST_DISCUSSIONS_DESCRIPTION", "List discussions for a repository")), diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 70e997ec3..78c1be725 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -318,7 +318,7 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) -// (Lines removed) + // (Lines removed) var response struct { Comments []*github.IssueComment `json:"comments"` From 985f8b234aca78a5f15581d0d526f541d8b34425 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 15:53:23 +0100 Subject: [PATCH 23/26] reduce default pagination size from 100 to 30 in discussion tests --- pkg/github/discussions_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 78c1be725..a017c9fa0 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -282,7 +282,7 @@ func Test_GetDiscussionComments(t *testing.T) { "owner": "owner", "repo": "repo", "discussionNumber": float64(1), - "first": float64(100), + "first": float64(30), "after": (*string)(nil), } @@ -344,7 +344,7 @@ func Test_ListDiscussionCategories(t *testing.T) { vars := map[string]interface{}{ "owner": "owner", "repo": "repo", - "first": float64(100), + "first": float64(30), "after": (*string)(nil), } From 5d795eccf5e93dd0abaab7efe3efaa1716882732 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Thu, 17 Jul 2025 16:36:47 +0100 Subject: [PATCH 24/26] update pagination for reverse and total --- pkg/github/discussions.go | 56 +++++++++++++++++++++++---------- pkg/github/discussions_test.go | 57 +++++++++++++++++++++++----------- 2 files changed, 79 insertions(+), 34 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 4e6837837..8b9ae22a2 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -92,9 +92,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp URL githubv4.String `graphql:"url"` } PageInfo struct { - HasNextPage bool - EndCursor string + HasNextPage bool + HasPreviousPage bool + StartCursor string + EndCursor string } + TotalCount int } `graphql:"discussions(first: $first, after: $after, categoryId: $categoryId)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -131,9 +134,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp response := map[string]interface{}{ "discussions": discussions, "pageInfo": map[string]interface{}{ - "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, - "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "hasPreviousPage": query.Repository.Discussions.PageInfo.HasPreviousPage, + "startCursor": query.Repository.Discussions.PageInfo.StartCursor, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, }, + "totalCount": query.Repository.Discussions.TotalCount, } out, err = json.Marshal(response) @@ -155,9 +161,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp URL githubv4.String `graphql:"url"` } PageInfo struct { - HasNextPage bool - EndCursor string + HasNextPage bool + HasPreviousPage bool + StartCursor string + EndCursor string } + TotalCount int } `graphql:"discussions(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -193,9 +202,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp response := map[string]interface{}{ "discussions": discussions, "pageInfo": map[string]interface{}{ - "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, - "endCursor": query.Repository.Discussions.PageInfo.EndCursor, + "hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage, + "hasPreviousPage": query.Repository.Discussions.PageInfo.HasPreviousPage, + "startCursor": query.Repository.Discussions.PageInfo.StartCursor, + "endCursor": query.Repository.Discussions.PageInfo.EndCursor, }, + "totalCount": query.Repository.Discussions.TotalCount, } out, err = json.Marshal(response) @@ -341,9 +353,12 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati Body githubv4.String } PageInfo struct { - HasNextPage githubv4.Boolean - EndCursor githubv4.String + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String } + TotalCount int } `graphql:"comments(first: $first, after: $after)"` } `graphql:"discussion(number: $discussionNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` @@ -372,9 +387,12 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati response := map[string]interface{}{ "comments": comments, "pageInfo": map[string]interface{}{ - "hasNextPage": q.Repository.Discussion.Comments.PageInfo.HasNextPage, - "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), + "hasNextPage": q.Repository.Discussion.Comments.PageInfo.HasNextPage, + "hasPreviousPage": q.Repository.Discussion.Comments.PageInfo.HasPreviousPage, + "startCursor": string(q.Repository.Discussion.Comments.PageInfo.StartCursor), + "endCursor": string(q.Repository.Discussion.Comments.PageInfo.EndCursor), }, + "totalCount": q.Repository.Discussion.Comments.TotalCount, } out, err := json.Marshal(response) @@ -448,9 +466,12 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl Name githubv4.String } PageInfo struct { - HasNextPage githubv4.Boolean - EndCursor githubv4.String + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String } + TotalCount int } `graphql:"discussionCategories(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -480,9 +501,12 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl response := map[string]interface{}{ "categories": categories, "pageInfo": map[string]interface{}{ - "hasNextPage": q.Repository.DiscussionCategories.PageInfo.HasNextPage, - "endCursor": string(q.Repository.DiscussionCategories.PageInfo.EndCursor), + "hasNextPage": q.Repository.DiscussionCategories.PageInfo.HasNextPage, + "hasPreviousPage": q.Repository.DiscussionCategories.PageInfo.HasPreviousPage, + "startCursor": string(q.Repository.DiscussionCategories.PageInfo.StartCursor), + "endCursor": string(q.Repository.DiscussionCategories.PageInfo.EndCursor), }, + "totalCount": q.Repository.DiscussionCategories.TotalCount, } out, err := json.Marshal(response) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index a017c9fa0..94cfbe337 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -30,9 +30,12 @@ var ( "discussions": map[string]any{ "nodes": discussionsAll, "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", }, + "totalCount": 3, }, }, }) @@ -41,9 +44,12 @@ var ( "discussions": map[string]any{ "nodes": discussionsGeneral, "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", }, + "totalCount": 2, }, }, }) @@ -61,9 +67,9 @@ func Test_ListDiscussions(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) // Use exact string queries that match implementation output (from error messages) - qDiscussions := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussions := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" - qDiscussionsFiltered := "query($after:String$categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}" + qDiscussionsFiltered := "query($after:String$categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, after: $after, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling varsListAll := map[string]interface{}{ @@ -161,9 +167,12 @@ func Test_ListDiscussions(t *testing.T) { var response struct { Discussions []*github.Discussion `json:"discussions"` PageInfo struct { - HasNextPage bool `json:"hasNextPage"` - EndCursor string `json:"endCursor"` + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + StartCursor string `json:"startCursor"` + EndCursor string `json:"endCursor"` } `json:"pageInfo"` + TotalCount int `json:"totalCount"` } err = json.Unmarshal([]byte(text), &response) require.NoError(t, err) @@ -275,7 +284,7 @@ func Test_GetDiscussionComments(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) // Use exact string query that matches implementation output - 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,endCursor}}}}}" + 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}}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ @@ -295,9 +304,12 @@ func Test_GetDiscussionComments(t *testing.T) { {"body": "This is the second comment"}, }, "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", }, + "totalCount": 2, }, }, }, @@ -323,9 +335,12 @@ func Test_GetDiscussionComments(t *testing.T) { var response struct { Comments []*github.IssueComment `json:"comments"` PageInfo struct { - HasNextPage bool `json:"hasNextPage"` - EndCursor string `json:"endCursor"` + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + StartCursor string `json:"startCursor"` + EndCursor string `json:"endCursor"` } `json:"pageInfo"` + TotalCount int `json:"totalCount"` } err = json.Unmarshal([]byte(textContent.Text), &response) require.NoError(t, err) @@ -338,7 +353,7 @@ func Test_GetDiscussionComments(t *testing.T) { func Test_ListDiscussionCategories(t *testing.T) { // Use exact string query that matches implementation output - qListCategories := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}" + qListCategories := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ @@ -356,9 +371,12 @@ func Test_ListDiscussionCategories(t *testing.T) { {"id": "456", "name": "CategoryTwo"}, }, "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", }, + "totalCount": 2, }, }, }) @@ -385,9 +403,12 @@ func Test_ListDiscussionCategories(t *testing.T) { var response struct { Categories []map[string]string `json:"categories"` PageInfo struct { - HasNextPage bool `json:"hasNextPage"` - EndCursor string `json:"endCursor"` + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + StartCursor string `json:"startCursor"` + EndCursor string `json:"endCursor"` } `json:"pageInfo"` + TotalCount int `json:"totalCount"` } require.NoError(t, json.Unmarshal([]byte(text), &response)) assert.Len(t, response.Categories, 2) From 7877d8db963ac950f1b9ba3ae88dfcb85b0d146b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 18 Jul 2025 09:52:36 +0100 Subject: [PATCH 25/26] update pagination to remove from discussions --- pkg/github/discussions.go | 45 +++++--------------------- pkg/github/discussions_test.go | 8 ++--- pkg/github/server.go | 58 ++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 43 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 8b9ae22a2..2b8ccfb0b 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -33,7 +33,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp mcp.WithString("category", mcp.Description("Optional filter by discussion category ID. If provided, only discussions with this category are listed."), ), - WithUnifiedPagination(), + WithCursorPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Required params @@ -53,7 +53,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } // Get pagination parameters and convert to GraphQL format - pagination, err := OptionalPaginationParams(request) + pagination, err := OptionalCursorPaginationParams(request) if err != nil { return nil, err } @@ -305,7 +305,7 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati mcp.WithString("owner", mcp.Required(), mcp.Description("Repository owner")), mcp.WithString("repo", mcp.Required(), mcp.Description("Repository name")), mcp.WithNumber("discussionNumber", mcp.Required(), mcp.Description("Discussion Number")), - WithUnifiedPagination(), + WithCursorPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -319,22 +319,21 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati } // Get pagination parameters and convert to GraphQL format - pagination, err := OptionalPaginationParams(request) + pagination, err := OptionalCursorPaginationParams(request) if err != nil { return nil, err } // Check if pagination parameters were explicitly provided - _, pageProvided := request.GetArguments()["page"] _, perPageProvided := request.GetArguments()["perPage"] - paginationExplicit := pageProvided || perPageProvided + paginationExplicit := perPageProvided paginationParams, err := pagination.ToGraphQLParams() if err != nil { return nil, err } - // Use default of 100 if pagination was not explicitly provided + // Use default of 30 if pagination was not explicitly provided if !paginationExplicit { defaultFirst := int32(DefaultGraphQLPageSize) paginationParams.First = &defaultFirst @@ -419,7 +418,6 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl mcp.Required(), mcp.Description("Repository name"), ), - WithUnifiedPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -431,28 +429,6 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl return mcp.NewToolResultError(err.Error()), nil } - // Get pagination parameters and convert to GraphQL format - pagination, err := OptionalPaginationParams(request) - if err != nil { - return nil, err - } - - // Check if pagination parameters were explicitly provided - _, pageProvided := request.GetArguments()["page"] - _, perPageProvided := request.GetArguments()["perPage"] - paginationExplicit := pageProvided || perPageProvided - - paginationParams, err := pagination.ToGraphQLParams() - if err != nil { - return nil, err - } - - // Use default of 100 if pagination was not explicitly provided - if !paginationExplicit { - defaultFirst := int32(DefaultGraphQLPageSize) - paginationParams.First = &defaultFirst - } - client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil @@ -472,18 +448,13 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl EndCursor githubv4.String } TotalCount int - } `graphql:"discussionCategories(first: $first, after: $after)"` + } `graphql:"discussionCategories(first: $first)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), - "first": githubv4.Int(*paginationParams.First), - } - if paginationParams.After != nil { - vars["after"] = githubv4.String(*paginationParams.After) - } else { - vars["after"] = (*githubv4.String)(nil) + "first": githubv4.Int(25), } if err := client.Query(ctx, &q, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 94cfbe337..e2e3d99ed 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -353,14 +353,13 @@ func Test_GetDiscussionComments(t *testing.T) { func Test_ListDiscussionCategories(t *testing.T) { // Use exact string query that matches implementation output - qListCategories := "query($after:String$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" + qListCategories := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first){nodes{id,name},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" // Variables matching what GraphQL receives after JSON marshaling/unmarshaling vars := map[string]interface{}{ "owner": "owner", "repo": "repo", - "first": float64(30), - "after": (*string)(nil), + "first": float64(25), } mockResp := githubv4mock.DataResponse(map[string]any{ @@ -397,9 +396,6 @@ func Test_ListDiscussionCategories(t *testing.T) { text := getTextResult(t, result).Text - // Debug: print the actual JSON response - t.Logf("JSON response: %s", text) - var response struct { Categories []map[string]string `json:"categories"` PageInfo struct { diff --git a/pkg/github/server.go b/pkg/github/server.go index 9b109c1f0..8269f717c 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -212,6 +212,21 @@ func WithUnifiedPagination() mcp.ToolOption { } } +// WithCursorPagination adds only cursor-based pagination parameters to a tool (no page parameter). +func WithCursorPagination() mcp.ToolOption { + return func(tool *mcp.Tool) { + mcp.WithNumber("perPage", + mcp.Description("Results per page for pagination (min 1, max 100)"), + mcp.Min(1), + mcp.Max(100), + )(tool) + + mcp.WithString("after", + mcp.Description("Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs."), + )(tool) + } +} + type PaginationParams struct { Page int PerPage int @@ -243,6 +258,49 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) { }, nil } +// OptionalCursorPaginationParams returns the "perPage" and "after" parameters from the request, +// without the "page" parameter, suitable for cursor-based pagination only. +func OptionalCursorPaginationParams(r mcp.CallToolRequest) (CursorPaginationParams, error) { + perPage, err := OptionalIntParamWithDefault(r, "perPage", 30) + if err != nil { + return CursorPaginationParams{}, err + } + after, err := OptionalParam[string](r, "after") + if err != nil { + return CursorPaginationParams{}, err + } + return CursorPaginationParams{ + PerPage: perPage, + After: after, + }, nil +} + +type CursorPaginationParams struct { + PerPage int + After string +} + +// ToGraphQLParams converts cursor pagination parameters to GraphQL-specific parameters. +func (p CursorPaginationParams) ToGraphQLParams() (GraphQLPaginationParams, error) { + if p.PerPage > 100 { + return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d exceeds maximum of 100", p.PerPage) + } + if p.PerPage < 0 { + return GraphQLPaginationParams{}, fmt.Errorf("perPage value %d cannot be negative", p.PerPage) + } + first := int32(p.PerPage) + + var after *string + if p.After != "" { + after = &p.After + } + + return GraphQLPaginationParams{ + First: &first, + After: after, + }, nil +} + type GraphQLPaginationParams struct { First *int32 After *string From f870807d49f8a62ce774b8d6f91edf11ffd08aff Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 18 Jul 2025 10:18:36 +0100 Subject: [PATCH 26/26] updated README --- README.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/README.md b/README.md index 6757e9bfa..e3f3208c3 100644 --- a/README.md +++ b/README.md @@ -584,22 +584,17 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `discussionNumber`: Discussion Number (number, required) - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories - - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_discussions** - List discussions - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required)