diff --git a/README.md b/README.md index acbf717f0..c8a55ea7b 100644 --- a/README.md +++ b/README.md @@ -539,15 +539,15 @@ The following sets of tools are available (all are on by default): - `repo`: Repository name (string, required) - **list_issues** - List issues - - `direction`: Sort direction (string, optional) + - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) + - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) - `labels`: Filter by labels (string[], optional) + - `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (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) - `since`: Filter by date (ISO 8601 timestamp) (string, optional) - - `sort`: Sort order (string, optional) - - `state`: Filter by state (string, optional) + - `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional) - **list_sub_issues** - List sub-issues - `issue_number`: Issue number (number, required) diff --git a/pkg/github/__toolsnaps__/list_issues.snap b/pkg/github/__toolsnaps__/list_issues.snap index 4fe155f09..f63da9c85 100644 --- a/pkg/github/__toolsnaps__/list_issues.snap +++ b/pkg/github/__toolsnaps__/list_issues.snap @@ -3,14 +3,18 @@ "title": "List issues", "readOnlyHint": true }, - "description": "List issues in a GitHub repository.", + "description": "List issues in a GitHub repository. For pagination, use the 'endCursor' from the previous response's 'pageInfo' in the 'after' parameter.", "inputSchema": { "properties": { + "after": { + "description": "Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs.", + "type": "string" + }, "direction": { - "description": "Sort direction", + "description": "Order direction. If provided, the 'orderBy' also needs to be provided.", "enum": [ - "asc", - "desc" + "ASC", + "DESC" ], "type": "string" }, @@ -21,15 +25,18 @@ }, "type": "array" }, + "orderBy": { + "description": "Order issues by field. If provided, the 'direction' also needs to be provided.", + "enum": [ + "CREATED_AT", + "UPDATED_AT" + ], + "type": "string" + }, "owner": { "description": "Repository owner", "type": "string" }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, "perPage": { "description": "Results per page for pagination (min 1, max 100)", "maximum": 100, @@ -44,21 +51,11 @@ "description": "Filter by date (ISO 8601 timestamp)", "type": "string" }, - "sort": { - "description": "Sort order", - "enum": [ - "created", - "updated", - "comments" - ], - "type": "string" - }, "state": { - "description": "Filter by state", + "description": "Filter by state, by default both open and closed issues are returned when not provided", "enum": [ - "open", - "closed", - "all" + "OPEN", + "CLOSED" ], "type": "string" } diff --git a/pkg/github/issues.go b/pkg/github/issues.go index de35dac12..ad0a0749b 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -18,6 +18,128 @@ import ( "github.com/shurcooL/githubv4" ) +// IssueFragment represents a fragment of an issue node in the GraphQL API. +type IssueFragment struct { + Number githubv4.Int + Title githubv4.String + Body githubv4.String + State githubv4.String + DatabaseID int64 + + Author struct { + Login githubv4.String + } + CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime + Labels struct { + Nodes []struct { + Name githubv4.String + ID githubv4.String + Description githubv4.String + } + } `graphql:"labels(first: 100)"` +} + +// Common interface for all issue query types +type IssueQueryResult interface { + GetIssueFragment() IssueQueryFragment +} + +type IssueQueryFragment struct { + Nodes []IssueFragment `graphql:"nodes"` + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + TotalCount int +} + +// ListIssuesQuery is the root query structure for fetching issues with optional label filtering. +type ListIssuesQuery struct { + Repository struct { + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +// ListIssuesQueryTypeWithLabels is the query structure for fetching issues with optional label filtering. +type ListIssuesQueryTypeWithLabels struct { + Repository struct { + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +// ListIssuesQueryWithSince is the query structure for fetching issues without label filtering but with since filtering. +type ListIssuesQueryWithSince struct { + Repository struct { + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +// ListIssuesQueryTypeWithLabelsWithSince is the query structure for fetching issues with both label and since filtering. +type ListIssuesQueryTypeWithLabelsWithSince struct { + Repository struct { + Issues IssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +// Implement the interface for all query types +func (q *ListIssuesQueryTypeWithLabels) GetIssueFragment() IssueQueryFragment { + return q.Repository.Issues +} + +func (q *ListIssuesQuery) GetIssueFragment() IssueQueryFragment { + return q.Repository.Issues +} + +func (q *ListIssuesQueryWithSince) GetIssueFragment() IssueQueryFragment { + return q.Repository.Issues +} + +func (q *ListIssuesQueryTypeWithLabelsWithSince) GetIssueFragment() IssueQueryFragment { + return q.Repository.Issues +} + +func getIssueQueryType(hasLabels bool, hasSince bool) any { + switch { + case hasLabels && hasSince: + return &ListIssuesQueryTypeWithLabelsWithSince{} + case hasLabels: + return &ListIssuesQueryTypeWithLabels{} + case hasSince: + return &ListIssuesQueryWithSince{} + default: + return &ListIssuesQuery{} + } +} + +func fragmentToIssue(fragment IssueFragment) *github.Issue { + // Convert GraphQL labels to GitHub API labels format + var foundLabels []*github.Label + for _, labelNode := range fragment.Labels.Nodes { + foundLabels = append(foundLabels, &github.Label{ + Name: github.Ptr(string(labelNode.Name)), + NodeID: github.Ptr(string(labelNode.ID)), + Description: github.Ptr(string(labelNode.Description)), + }) + } + + return &github.Issue{ + Number: github.Ptr(int(fragment.Number)), + Title: github.Ptr(string(fragment.Title)), + CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time}, + UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time}, + User: &github.User{ + Login: github.Ptr(string(fragment.Author.Login)), + }, + State: github.Ptr(string(fragment.State)), + ID: github.Ptr(fragment.DatabaseID), + Body: github.Ptr(string(fragment.Body)), + Labels: foundLabels, + } +} + // GetIssue creates a tool to get details of a specific issue in a GitHub repository. func GetIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_issue", @@ -724,9 +846,9 @@ func CreateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } // ListIssues creates a tool to list and filter repository issues -func ListIssues(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func ListIssues(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("list_issues", - mcp.WithDescription(t("TOOL_LIST_ISSUES_DESCRIPTION", "List issues in a GitHub repository.")), + mcp.WithDescription(t("TOOL_LIST_ISSUES_DESCRIPTION", "List issues in a GitHub repository. For pagination, use the 'endCursor' from the previous response's 'pageInfo' in the 'after' parameter.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_LIST_ISSUES_USER_TITLE", "List issues"), ReadOnlyHint: ToBoolPtr(true), @@ -740,8 +862,8 @@ func ListIssues(getClient GetClientFn, t translations.TranslationHelperFunc) (to mcp.Description("Repository name"), ), mcp.WithString("state", - mcp.Description("Filter by state"), - mcp.Enum("open", "closed", "all"), + mcp.Description("Filter by state, by default both open and closed issues are returned when not provided"), + mcp.Enum("OPEN", "CLOSED"), ), mcp.WithArray("labels", mcp.Description("Filter by labels"), @@ -751,18 +873,18 @@ func ListIssues(getClient GetClientFn, t translations.TranslationHelperFunc) (to }, ), ), - mcp.WithString("sort", - mcp.Description("Sort order"), - mcp.Enum("created", "updated", "comments"), + mcp.WithString("orderBy", + mcp.Description("Order issues by field. If provided, the 'direction' also needs to be provided."), + mcp.Enum("CREATED_AT", "UPDATED_AT"), ), mcp.WithString("direction", - mcp.Description("Sort direction"), - mcp.Enum("asc", "desc"), + mcp.Description("Order direction. If provided, the 'orderBy' also needs to be provided."), + mcp.Enum("ASC", "DESC"), ), mcp.WithString("since", mcp.Description("Filter by date (ISO 8601 timestamp)"), ), - WithPagination(), + WithCursorPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := RequiredParam[string](request, "owner") @@ -774,74 +896,164 @@ func ListIssues(getClient GetClientFn, t translations.TranslationHelperFunc) (to return mcp.NewToolResultError(err.Error()), nil } - opts := &github.IssueListByRepoOptions{} - // Set optional parameters if provided - opts.State, err = OptionalParam[string](request, "state") + state, err := OptionalParam[string](request, "state") if err != nil { return mcp.NewToolResultError(err.Error()), nil } + // If the state has a value, cast into an array of strings + var states []githubv4.IssueState + if state != "" { + states = append(states, githubv4.IssueState(state)) + } else { + states = []githubv4.IssueState{githubv4.IssueStateOpen, githubv4.IssueStateClosed} + } + // Get labels - opts.Labels, err = OptionalStringArrayParam(request, "labels") + labels, err := OptionalStringArrayParam(request, "labels") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - opts.Sort, err = OptionalParam[string](request, "sort") + orderBy, err := OptionalParam[string](request, "orderBy") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - opts.Direction, err = OptionalParam[string](request, "direction") + direction, err := OptionalParam[string](request, "direction") if err != nil { return mcp.NewToolResultError(err.Error()), nil } + // These variables are required for the GraphQL query to be set by default + // If orderBy is empty, default to CREATED_AT + if orderBy == "" { + orderBy = "CREATED_AT" + } + // If direction is empty, default to DESC + if direction == "" { + direction = "DESC" + } + since, err := OptionalParam[string](request, "since") if err != nil { return mcp.NewToolResultError(err.Error()), nil } + + // There are two optional parameters: since and labels. + var sinceTime time.Time + var hasSince bool if since != "" { - timestamp, err := parseISOTimestamp(since) + sinceTime, err = parseISOTimestamp(since) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to list issues: %s", err.Error())), nil } - opts.Since = timestamp + hasSince = true } + hasLabels := len(labels) > 0 - if page, ok := request.GetArguments()["page"].(float64); ok { - opts.ListOptions.Page = int(page) + // Get pagination parameters and convert to GraphQL format + pagination, err := OptionalCursorPaginationParams(request) + if err != nil { + return nil, err } - if perPage, ok := request.GetArguments()["perPage"].(float64); ok { - opts.ListOptions.PerPage = int(perPage) + // Check if someone tried to use page-based pagination instead of cursor-based + if _, pageProvided := request.GetArguments()["page"]; pageProvided { + return mcp.NewToolResultError("This tool uses cursor-based pagination. Use the 'after' parameter with the 'endCursor' value from the previous response instead of 'page'."), nil } - client, err := getClient(ctx) + // Check if pagination parameters were explicitly provided + _, perPageProvided := request.GetArguments()["perPage"] + paginationExplicit := perPageProvided + + paginationParams, err := pagination.ToGraphQLParams() if err != nil { - return nil, fmt.Errorf("failed to get GitHub client: %w", err) + return nil, err + } + + // Use default of 30 if pagination was not explicitly provided + if !paginationExplicit { + defaultFirst := int32(DefaultGraphQLPageSize) + paginationParams.First = &defaultFirst } - issues, resp, err := client.Issues.ListByRepo(ctx, owner, repo, opts) + + client, err := getGQLClient(ctx) if err != nil { - return nil, fmt.Errorf("failed to list issues: %w", err) + return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + vars := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "states": states, + "orderBy": githubv4.IssueOrderField(orderBy), + "direction": githubv4.OrderDirection(direction), + "first": githubv4.Int(*paginationParams.First), + } + + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + // Used within query, therefore must be set to nil and provided as $after + vars["after"] = (*githubv4.String)(nil) + } + + // Ensure optional parameters are set + if hasLabels { + // Use query with labels filtering - convert string labels to githubv4.String slice + labelStrings := make([]githubv4.String, len(labels)) + for i, label := range labels { + labelStrings[i] = githubv4.String(label) } - return mcp.NewToolResultError(fmt.Sprintf("failed to list issues: %s", string(body))), nil + vars["labels"] = labelStrings } - r, err := json.Marshal(issues) + if hasSince { + vars["since"] = githubv4.DateTime{Time: sinceTime} + } + + issueQuery := getIssueQueryType(hasLabels, hasSince) + if err := client.Query(ctx, issueQuery, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + // Extract and convert all issue nodes using the common interface + var issues []*github.Issue + var pageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + var totalCount int + + if queryResult, ok := issueQuery.(IssueQueryResult); ok { + fragment := queryResult.GetIssueFragment() + for _, issue := range fragment.Nodes { + issues = append(issues, fragmentToIssue(issue)) + } + pageInfo = fragment.PageInfo + totalCount = fragment.TotalCount + } + + // Create response with issues + response := map[string]interface{}{ + "issues": issues, + "pageInfo": map[string]interface{}{ + "hasNextPage": pageInfo.HasNextPage, + "hasPreviousPage": pageInfo.HasPreviousPage, + "startCursor": string(pageInfo.StartCursor), + "endCursor": string(pageInfo.EndCursor), + }, + "totalCount": totalCount, + } + out, err := json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal issues: %w", err) } - - return mcp.NewToolResultText(string(r)), nil + return mcp.NewToolResultText(string(out)), nil } } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3de27c8e6..2a530ef48 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -648,8 +648,8 @@ func Test_CreateIssue(t *testing.T) { func Test_ListIssues(t *testing.T) { // Verify tool definition - mockClient := github.NewClient(nil) - tool, _ := ListIssues(stubGetClientFn(mockClient), translations.NullTranslationHelper) + mockClient := githubv4.NewClient(nil) + tool, _ := ListIssues(stubGetGQLClientFn(mockClient), translations.NullTranslationHelper) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "list_issues", tool.Name) @@ -658,166 +658,288 @@ func Test_ListIssues(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "repo") assert.Contains(t, tool.InputSchema.Properties, "state") assert.Contains(t, tool.InputSchema.Properties, "labels") - assert.Contains(t, tool.InputSchema.Properties, "sort") + assert.Contains(t, tool.InputSchema.Properties, "orderBy") assert.Contains(t, tool.InputSchema.Properties, "direction") assert.Contains(t, tool.InputSchema.Properties, "since") - assert.Contains(t, tool.InputSchema.Properties, "page") + assert.Contains(t, tool.InputSchema.Properties, "after") assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo"}) - // Setup mock issues for success case - mockIssues := []*github.Issue{ + // Mock issues data + mockIssuesAll := []map[string]any{ + { + "number": 123, + "title": "First Issue", + "body": "This is the first test issue", + "state": "OPEN", + "databaseId": 1001, + "createdAt": "2023-01-01T00:00:00Z", + "updatedAt": "2023-01-01T00:00:00Z", + "author": map[string]any{"login": "user1"}, + "labels": map[string]any{ + "nodes": []map[string]any{ + {"name": "bug", "id": "label1", "description": "Bug label"}, + }, + }, + }, { - Number: github.Ptr(123), - Title: github.Ptr("First Issue"), - Body: github.Ptr("This is the first test issue"), - State: github.Ptr("open"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - CreatedAt: &github.Timestamp{Time: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC)}, + "number": 456, + "title": "Second Issue", + "body": "This is the second test issue", + "state": "OPEN", + "databaseId": 1002, + "createdAt": "2023-02-01T00:00:00Z", + "updatedAt": "2023-02-01T00:00:00Z", + "author": map[string]any{"login": "user2"}, + "labels": map[string]any{ + "nodes": []map[string]any{ + {"name": "enhancement", "id": "label2", "description": "Enhancement label"}, + }, + }, }, + } + + mockIssuesOpen := []map[string]any{mockIssuesAll[0], mockIssuesAll[1]} + mockIssuesClosed := []map[string]any{ { - Number: github.Ptr(456), - Title: github.Ptr("Second Issue"), - Body: github.Ptr("This is the second test issue"), - State: github.Ptr("open"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/456"), - Labels: []*github.Label{{Name: github.Ptr("bug")}}, - CreatedAt: &github.Timestamp{Time: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC)}, + "number": 789, + "title": "Closed Issue", + "body": "This is a closed issue", + "state": "CLOSED", + "databaseId": 1003, + "createdAt": "2023-03-01T00:00:00Z", + "updatedAt": "2023-03-01T00:00:00Z", + "author": map[string]any{"login": "user3"}, + "labels": map[string]any{ + "nodes": []map[string]any{}, + }, }, } + // Mock responses + mockResponseListAll := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issues": map[string]any{ + "nodes": mockIssuesAll, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, + }, + }, + }) + + mockResponseOpenOnly := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issues": map[string]any{ + "nodes": mockIssuesOpen, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 2, + }, + }, + }) + + mockResponseClosedOnly := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issues": map[string]any{ + "nodes": mockIssuesClosed, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 1, + }, + }, + }) + + mockErrorRepoNotFound := githubv4mock.ErrorResponse("repository not found") + + // Variables matching what GraphQL receives after JSON marshaling/unmarshaling + varsListAll := map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "states": []interface{}{"OPEN", "CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + + varsOpenOnly := map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "states": []interface{}{"OPEN"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + + varsClosedOnly := map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "states": []interface{}{"CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + + varsWithLabels := map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "states": []interface{}{"OPEN", "CLOSED"}, + "labels": []interface{}{"bug", "enhancement"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + + varsRepoNotFound := map[string]interface{}{ + "owner": "owner", + "repo": "nonexistent-repo", + "states": []interface{}{"OPEN", "CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + } + tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]interface{} - expectError bool - expectedIssues []*github.Issue - expectedErrMsg string + name string + reqParams map[string]interface{} + expectError bool + errContains string + expectedCount int + verifyOrder func(t *testing.T, issues []*github.Issue) }{ { - name: "list issues with minimal parameters", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepo, - mockIssues, - ), - ), - requestArgs: map[string]interface{}{ + name: "list all issues", + reqParams: map[string]interface{}{ "owner": "owner", "repo": "repo", }, - expectError: false, - expectedIssues: mockIssues, + expectError: false, + expectedCount: 2, }, { - name: "list issues with all parameters", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposIssuesByOwnerByRepo, - expectQueryParams(t, map[string]string{ - "state": "open", - "labels": "bug,enhancement", - "sort": "created", - "direction": "desc", - "since": "2023-01-01T00:00:00Z", - "page": "1", - "per_page": "30", - }).andThen( - mockResponse(t, http.StatusOK, mockIssues), - ), - ), - ), - requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "state": "open", - "labels": []any{"bug", "enhancement"}, - "sort": "created", - "direction": "desc", - "since": "2023-01-01T00:00:00Z", - "page": float64(1), - "perPage": float64(30), + name: "filter by open state", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "state": "OPEN", }, - expectError: false, - expectedIssues: mockIssues, + expectError: false, + expectedCount: 2, }, { - name: "invalid since parameter", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepo, - mockIssues, - ), - ), - requestArgs: map[string]interface{}{ + name: "filter by closed state", + reqParams: map[string]interface{}{ "owner": "owner", "repo": "repo", - "since": "invalid-date", + "state": "CLOSED", }, - expectError: true, - expectedErrMsg: "invalid ISO 8601 timestamp", + expectError: false, + expectedCount: 1, }, { - name: "list issues fails with error", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposIssuesByOwnerByRepo, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Repository not found"}`)) - }), - ), - ), - requestArgs: map[string]interface{}{ - "owner": "nonexistent", - "repo": "repo", + name: "filter by labels", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "labels": []any{"bug", "enhancement"}, }, - expectError: true, - expectedErrMsg: "failed to list issues", + expectError: false, + expectedCount: 2, + }, + { + name: "repository not found error", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "nonexistent-repo", + }, + expectError: true, + errContains: "repository not found", }, } + // Define the actual query strings that match the implementation + qBasicNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" + qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}" + for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Setup client with mock - client := github.NewClient(tc.mockedClient) - _, handler := ListIssues(stubGetClientFn(client), translations.NullTranslationHelper) + var httpClient *http.Client + + switch tc.name { + case "list all issues": + matcher := githubv4mock.NewQueryMatcher(qBasicNoLabels, varsListAll, mockResponseListAll) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "filter by open state": + matcher := githubv4mock.NewQueryMatcher(qBasicNoLabels, varsOpenOnly, mockResponseOpenOnly) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "filter by closed state": + matcher := githubv4mock.NewQueryMatcher(qBasicNoLabels, varsClosedOnly, mockResponseClosedOnly) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "filter by labels": + matcher := githubv4mock.NewQueryMatcher(qWithLabels, varsWithLabels, mockResponseListAll) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "repository not found error": + matcher := githubv4mock.NewQueryMatcher(qBasicNoLabels, varsRepoNotFound, mockErrorRepoNotFound) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + } - // Create call request - request := createMCPRequest(tc.requestArgs) + gqlClient := githubv4.NewClient(httpClient) + _, handler := ListIssues(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) - // Call handler - result, err := handler(context.Background(), request) + req := createMCPRequest(tc.reqParams) + res, err := handler(context.Background(), req) + text := getTextResult(t, res).Text - // Verify results if tc.expectError { - if err != nil { - assert.Contains(t, err.Error(), tc.expectedErrMsg) - } else { - // For errors returned as part of the result, not as an error - assert.NotNil(t, result) - textContent := getTextResult(t, result) - assert.Contains(t, textContent.Text, tc.expectedErrMsg) - } + require.True(t, res.IsError) + assert.Contains(t, text, tc.errContains) return } + require.NoError(t, err) + // Parse the structured response with pagination info + var response struct { + Issues []*github.Issue `json:"issues"` + 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) - // Parse the result and get the text content if no error - textContent := getTextResult(t, result) + assert.Len(t, response.Issues, tc.expectedCount, "Expected %d issues, got %d", tc.expectedCount, len(response.Issues)) - // Unmarshal and verify the result - var returnedIssues []*github.Issue - err = json.Unmarshal([]byte(textContent.Text), &returnedIssues) - require.NoError(t, err) + // Verify order if verifyOrder function is provided + if tc.verifyOrder != nil { + tc.verifyOrder(t, response.Issues) + } - assert.Len(t, returnedIssues, len(tc.expectedIssues)) - for i, issue := range returnedIssues { - assert.Equal(t, *tc.expectedIssues[i].Number, *issue.Number) - assert.Equal(t, *tc.expectedIssues[i].Title, *issue.Title) - assert.Equal(t, *tc.expectedIssues[i].State, *issue.State) - assert.Equal(t, *tc.expectedIssues[i].HTMLURL, *issue.HTMLURL) + // Verify that returned issues have expected structure + for _, issue := range response.Issues { + assert.NotNil(t, issue.Number, "Issue should have number") + assert.NotNil(t, issue.Title, "Issue should have title") + assert.NotNil(t, issue.State, "Issue should have state") } }) } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index f293b4aa5..b41ba9467 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -51,7 +51,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG AddReadTools( toolsets.NewServerTool(GetIssue(getClient, t)), toolsets.NewServerTool(SearchIssues(getClient, t)), - toolsets.NewServerTool(ListIssues(getClient, t)), + toolsets.NewServerTool(ListIssues(getGQLClient, t)), toolsets.NewServerTool(GetIssueComments(getClient, t)), toolsets.NewServerTool(ListSubIssues(getClient, t)), ).