diff --git a/README.md b/README.md index ae4d3627e..c06142b76 100644 --- a/README.md +++ b/README.md @@ -449,21 +449,21 @@ The following sets of tools are available (all are on by default): - `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) + - `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 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) - `owner`: Repository owner (string, required) - `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) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) 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 23e2724d4..2b8ccfb0b 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")), @@ -31,6 +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."), ), + WithCursorPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Required params @@ -49,6 +52,16 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } + // Get pagination parameters and convert to GraphQL format + pagination, err := OptionalCursorPaginationParams(request) + if err != nil { + return nil, err + } + paginationParams, err := pagination.ToGraphQLParams() + if err != nil { + return nil, err + } + client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil @@ -61,7 +74,8 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp categoryID = &id } - // Now execute the discussions query + var out []byte + var discussions []*github.Discussion if categoryID != nil { // Query with category filter (server-side filtering) @@ -77,13 +91,26 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` + PageInfo struct { + HasNextPage bool + HasPreviousPage bool + StartCursor string + EndCursor string + } + TotalCount int + } `graphql:"discussions(first: $first, after: $after, 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(*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 @@ -102,6 +129,23 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } discussions = append(discussions, di) } + + // Create response with pagination info + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "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) + if err != nil { + return nil, fmt.Errorf("failed to marshal discussions: %w", err) + } } else { // Query without category filter var query struct { @@ -116,12 +160,25 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100)"` + PageInfo struct { + HasNextPage bool + HasPreviousPage bool + StartCursor string + EndCursor string + } + TotalCount int + } `graphql:"discussions(first: $first, after: $after)"` } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]interface{}{ "owner": githubv4.String(owner), "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 @@ -140,13 +197,25 @@ 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) + // Create response with pagination info + response := map[string]interface{}{ + "discussions": discussions, + "pageInfo": map[string]interface{}{ + "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) + if err != nil { + return nil, fmt.Errorf("failed to marshal discussions: %w", err) + } } + return mcp.NewToolResultText(string(out)), nil } } @@ -236,6 +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")), + WithCursorPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Decode params @@ -248,6 +318,27 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati return mcp.NewToolResultError(err.Error()), nil } + // Get pagination parameters and convert to GraphQL format + pagination, err := OptionalCursorPaginationParams(request) + if err != nil { + return nil, err + } + + // Check if pagination parameters were explicitly provided + _, perPageProvided := request.GetArguments()["perPage"] + paginationExplicit := perPageProvided + + paginationParams, err := pagination.ToGraphQLParams() + if err != nil { + return nil, err + } + + // Use default of 30 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 @@ -260,7 +351,14 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati Nodes []struct { Body githubv4.String } - } `graphql:"comments(first:100)"` + PageInfo struct { + 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)"` } @@ -268,16 +366,35 @@ 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), + } + 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 } + 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) + // Create response with pagination info + response := map[string]interface{}{ + "comments": comments, + "pageInfo": map[string]interface{}{ + "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) if err != nil { return nil, fmt.Errorf("failed to marshal comments: %w", err) } @@ -301,55 +418,22 @@ 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"), - ), ), 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 - } - if params.Before != "" && params.First != 0 { - return mcp.NewToolResultError("'before' cannot be used with 'first'. Did you mean to use 'after' instead?"), nil - } - 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 { @@ -357,16 +441,25 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl ID githubv4.ID Name githubv4.String } - } `graphql:"discussionCategories(first: 100)"` + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + TotalCount int + } `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(25), } 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{ @@ -374,7 +467,20 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl "name": string(c.Name), }) } - 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, + "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) 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 c6688a519..e2e3d99ed 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -27,12 +27,30 @@ 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, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 3, + }, }, }) 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, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, + }, }, }) mockErrorRepoNotFound = githubv4mock.ErrorResponse("repository not found") @@ -48,54 +66,32 @@ 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$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}}}" - // 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$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{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), + "owner": "owner", + "repo": "repo", + "first": float64(30), + "after": (*string)(nil), } varsRepoNotFound := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("nonexistent-repo"), + "owner": "owner", + "repo": "nonexistent-repo", + "first": float64(30), + "after": (*string)(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), + "after": (*string)(nil), } tests := []struct { @@ -167,15 +163,25 @@ func Test_ListDiscussions(t *testing.T) { } require.NoError(t, err) - var returnedDiscussions []*github.Discussion - err = json.Unmarshal([]byte(text), &returnedDiscussions) + // Parse the structured response with pagination info + var response struct { + Discussions []*github.Discussion `json:"discussions"` + PageInfo struct { + 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) - 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") } @@ -194,23 +200,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 @@ -250,7 +246,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) @@ -287,22 +283,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,hasPreviousPage,startCursor,endCursor},totalCount}}}}" + + // 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), + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + "first": float64(30), + "after": (*string)(nil), } + mockResponse := githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "discussion": map[string]any{ @@ -311,11 +303,18 @@ func Test_GetDiscussionComments(t *testing.T) { {"body": "This is the first comment"}, {"body": "This is the second comment"}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, }, }, }, }) - 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) @@ -331,31 +330,38 @@ func Test_GetDiscussionComments(t *testing.T) { textContent := getTextResult(t, result) - var returnedComments []*github.IssueComment - err = json.Unmarshal([]byte(textContent.Text), &returnedComments) + // (Lines removed) + + var response struct { + Comments []*github.IssueComment `json:"comments"` + PageInfo struct { + 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) - 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) } } 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($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": githubv4.String("owner"), - "repo": githubv4.String("repo"), + "owner": "owner", + "repo": "repo", + "first": float64(25), } + mockResp := githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "discussionCategories": map[string]any{ @@ -363,10 +369,17 @@ func Test_ListDiscussionCategories(t *testing.T) { {"id": "123", "name": "CategoryOne"}, {"id": "456", "name": "CategoryTwo"}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, }, }, }) - matcher := githubv4mock.NewQueryMatcher(q, vars, mockResp) + matcher := githubv4mock.NewQueryMatcher(qListCategories, vars, mockResp) httpClient := githubv4mock.NewMockedHTTPClient(matcher) gqlClient := githubv4.NewClient(httpClient) @@ -382,11 +395,21 @@ 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 struct { + 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) + 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/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 d98dc334d..47b7c6bd2 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 2e56c8644..ecd36d7e0 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, }, } @@ -1198,8 +1198,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 ea476e3ac..193336b75 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,12 +191,49 @@ func WithPagination() mcp.ToolOption { } } +// 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) { + 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) + + mcp.WithString("after", + mcp.Description("Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs."), + )(tool) + } +} + +// 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 + 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 @@ -212,12 +247,77 @@ 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, + Page: page, + PerPage: perPage, + After: after, + }, 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 nil, fmt.Errorf("perPage value %d exceeds maximum of 100", p.PerPage) + } + if p.PerPage < 0 { + return nil, 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 +} + +// 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) { + // Convert to CursorPaginationParams and delegate to avoid duplication + cursor := CursorPaginationParams{ + PerPage: p.PerPage, + After: p.After, + } + return cursor.ToGraphQLParams() +} + 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, },