Content-Length: 24444 | pFad | http://github.com/github/github-mcp-server/pull/583.diff
thub.com diff --git a/pkg/github/__toolsnaps__/search_issues.snap b/pkg/github/__toolsnaps__/search_issues.snap index 4e2382a3c..7db502d94 100644 --- a/pkg/github/__toolsnaps__/search_issues.snap +++ b/pkg/github/__toolsnaps__/search_issues.snap @@ -3,7 +3,7 @@ "title": "Search issues", "readOnlyHint": true }, - "description": "Search for issues in GitHub repositories.", + "description": "Search for issues in GitHub repositories using issues search syntax already scoped to is:issue", "inputSchema": { "properties": { "order": { @@ -14,6 +14,10 @@ ], "type": "string" }, + "owner": { + "description": "Optional repository owner. If provided with repo, only notifications for this repository are listed.", + "type": "string" + }, "page": { "description": "Page number for pagination (min 1)", "minimum": 1, @@ -25,10 +29,14 @@ "minimum": 1, "type": "number" }, - "q": { + "query": { "description": "Search query using GitHub issues search syntax", "type": "string" }, + "repo": { + "description": "Optional repository name. If provided with owner, only notifications for this repository are listed.", + "type": "string" + }, "sort": { "description": "Sort field by number of matches of categories, defaults to best match", "enum": [ @@ -48,7 +56,7 @@ } }, "required": [ - "q" + "query" ], "type": "object" }, diff --git a/pkg/github/__toolsnaps__/search_pull_requests.snap b/pkg/github/__toolsnaps__/search_pull_requests.snap new file mode 100644 index 000000000..6a8d8e0e6 --- /dev/null +++ b/pkg/github/__toolsnaps__/search_pull_requests.snap @@ -0,0 +1,64 @@ +{ + "annotations": { + "title": "Search pull requests", + "readOnlyHint": true + }, + "description": "Search for pull requests in GitHub repositories using issues search syntax already scoped to is:pr", + "inputSchema": { + "properties": { + "order": { + "description": "Sort order", + "enum": [ + "asc", + "desc" + ], + "type": "string" + }, + "owner": { + "description": "Optional repository owner. If provided with repo, only notifications for this repository are listed.", + "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, + "minimum": 1, + "type": "number" + }, + "query": { + "description": "Search query using GitHub pull request search syntax", + "type": "string" + }, + "repo": { + "description": "Optional repository name. If provided with owner, only notifications for this repository are listed.", + "type": "string" + }, + "sort": { + "description": "Sort field by number of matches of categories, defaults to best match", + "enum": [ + "comments", + "reactions", + "reactions-+1", + "reactions--1", + "reactions-smile", + "reactions-thinking_face", + "reactions-heart", + "reactions-tada", + "interactions", + "created", + "updated" + ], + "type": "string" + } + }, + "required": [ + "query" + ], + "type": "object" + }, + "name": "search_pull_requests" +} \ No newline at end of file diff --git a/pkg/github/issues.go b/pkg/github/issues.go index b4c64c8de..3242c2be9 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -153,18 +153,24 @@ func AddIssueComment(getClient GetClientFn, t translations.TranslationHelperFunc } } -// SearchIssues creates a tool to search for issues and pull requests. +// SearchIssues creates a tool to search for issues. func SearchIssues(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("search_issues", - mcp.WithDescription(t("TOOL_SEARCH_ISSUES_DESCRIPTION", "Search for issues in GitHub repositories.")), + mcp.WithDescription(t("TOOL_SEARCH_ISSUES_DESCRIPTION", "Search for issues in GitHub repositories using issues search syntax already scoped to is:issue")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_SEARCH_ISSUES_USER_TITLE", "Search issues"), ReadOnlyHint: ToBoolPtr(true), }), - mcp.WithString("q", + mcp.WithString("query", mcp.Required(), mcp.Description("Search query using GitHub issues search syntax"), ), + mcp.WithString("owner", + mcp.Description("Optional repository owner. If provided with repo, only notifications for this repository are listed."), + ), + mcp.WithString("repo", + mcp.Description("Optional repository name. If provided with owner, only notifications for this repository are listed."), + ), mcp.WithString("sort", mcp.Description("Sort field by number of matches of categories, defaults to best match"), mcp.Enum( @@ -188,56 +194,7 @@ func SearchIssues(getClient GetClientFn, t translations.TranslationHelperFunc) ( WithPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - query, err := RequiredParam[string](request, "q") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - sort, err := OptionalParam[string](request, "sort") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - order, err := OptionalParam[string](request, "order") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - pagination, err := OptionalPaginationParams(request) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - opts := &github.SearchOptions{ - Sort: sort, - Order: order, - ListOptions: github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, - }, - } - - client, err := getClient(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get GitHub client: %w", err) - } - result, resp, err := client.Search.Issues(ctx, query, opts) - if err != nil { - return nil, fmt.Errorf("failed to search issues: %w", err) - } - 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) - } - return mcp.NewToolResultError(fmt.Sprintf("failed to search issues: %s", string(body))), nil - } - - r, err := json.Marshal(result) - if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) - } - - return mcp.NewToolResultText(string(r)), nil + return searchHandler(ctx, getClient, request, "issue", "failed to search issues") } } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 7c76d90f9..056fa7ed8 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -237,12 +237,14 @@ func Test_SearchIssues(t *testing.T) { assert.Equal(t, "search_issues", tool.Name) assert.NotEmpty(t, tool.Description) - assert.Contains(t, tool.InputSchema.Properties, "q") + assert.Contains(t, tool.InputSchema.Properties, "query") + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") assert.Contains(t, tool.InputSchema.Properties, "sort") assert.Contains(t, tool.InputSchema.Properties, "order") assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.Contains(t, tool.InputSchema.Properties, "page") - assert.ElementsMatch(t, tool.InputSchema.Required, []string{"q"}) + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"query"}) // Setup mock search results mockSearchResult := &github.IssuesSearchResult{ @@ -290,7 +292,7 @@ func Test_SearchIssues(t *testing.T) { expectQueryParams( t, map[string]string{ - "q": "repo:owner/repo is:issue is:open", + "q": "is:issue repo:owner/repo is:open", "sort": "created", "order": "desc", "page": "1", @@ -302,7 +304,7 @@ func Test_SearchIssues(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "q": "repo:owner/repo is:issue is:open", + "query": "repo:owner/repo is:open", "sort": "created", "order": "desc", "page": float64(1), @@ -311,6 +313,83 @@ func Test_SearchIssues(t *testing.T) { expectError: false, expectedResult: mockSearchResult, }, + { + name: "issues search with owner and repo parameters", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "repo:test-owner/test-repo is:issue is:open", + "sort": "created", + "order": "asc", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "is:open", + "owner": "test-owner", + "repo": "test-repo", + "sort": "created", + "order": "asc", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "issues search with only owner parameter (should ignore it)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "is:issue bug", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "bug", + "owner": "test-owner", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "issues search with only repo parameter (should ignore it)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "is:issue feature", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "feature", + "repo": "test-repo", + }, + expectError: false, + expectedResult: mockSearchResult, + }, { name: "issues search with minimal parameters", mockedClient: mock.NewMockedHTTPClient( @@ -320,7 +399,7 @@ func Test_SearchIssues(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "q": "repo:owner/repo is:issue is:open", + "query": "is:issue repo:owner/repo is:open", }, expectError: false, expectedResult: mockSearchResult, @@ -337,7 +416,7 @@ func Test_SearchIssues(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "q": "invalid:query", + "query": "invalid:query", }, expectError: true, expectedErrMsg: "failed to search issues", diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index d8f424673..bad822b13 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -533,6 +533,51 @@ func MergePullRequest(getClient GetClientFn, t translations.TranslationHelperFun } } +// SearchPullRequests creates a tool to search for pull requests. +func SearchPullRequests(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { + return mcp.NewTool("search_pull_requests", + mcp.WithDescription(t("TOOL_SEARCH_PULL_REQUESTS_DESCRIPTION", "Search for pull requests in GitHub repositories using issues search syntax already scoped to is:pr")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_SEARCH_PULL_REQUESTS_USER_TITLE", "Search pull requests"), + ReadOnlyHint: ToBoolPtr(true), + }), + mcp.WithString("query", + mcp.Required(), + mcp.Description("Search query using GitHub pull request search syntax"), + ), + mcp.WithString("owner", + mcp.Description("Optional repository owner. If provided with repo, only notifications for this repository are listed."), + ), + mcp.WithString("repo", + mcp.Description("Optional repository name. If provided with owner, only notifications for this repository are listed."), + ), + mcp.WithString("sort", + mcp.Description("Sort field by number of matches of categories, defaults to best match"), + mcp.Enum( + "comments", + "reactions", + "reactions-+1", + "reactions--1", + "reactions-smile", + "reactions-thinking_face", + "reactions-heart", + "reactions-tada", + "interactions", + "created", + "updated", + ), + ), + mcp.WithString("order", + mcp.Description("Sort order"), + mcp.Enum("asc", "desc"), + ), + WithPagination(), + ), + func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return searchHandler(ctx, getClient, request, "pr", "failed to search pull requests") + } +} + // GetPullRequestFiles creates a tool to get the list of files changed in a pull request. func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("get_pull_request_files", diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index e0966f805..30341e86c 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -565,6 +565,241 @@ func Test_MergePullRequest(t *testing.T) { } } +func Test_SearchPullRequests(t *testing.T) { + mockClient := github.NewClient(nil) + tool, _ := SearchPullRequests(stubGetClientFn(mockClient), translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "search_pull_requests", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "query") + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "sort") + assert.Contains(t, tool.InputSchema.Properties, "order") + assert.Contains(t, tool.InputSchema.Properties, "perPage") + assert.Contains(t, tool.InputSchema.Properties, "page") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"query"}) + + mockSearchResult := &github.IssuesSearchResult{ + Total: github.Ptr(2), + IncompleteResults: github.Ptr(false), + Issues: []*github.Issue{ + { + Number: github.Ptr(42), + Title: github.Ptr("Test PR 1"), + Body: github.Ptr("Updated tests."), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/1"), + Comments: github.Ptr(5), + User: &github.User{ + Login: github.Ptr("user1"), + }, + }, + { + Number: github.Ptr(43), + Title: github.Ptr("Test PR 2"), + Body: github.Ptr("Updated build scripts."), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/2"), + Comments: github.Ptr(3), + User: &github.User{ + Login: github.Ptr("user2"), + }, + }, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedResult *github.IssuesSearchResult + expectedErrMsg string + }{ + { + name: "successful pull request search with all parameters", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "is:pr repo:owner/repo is:open", + "sort": "created", + "order": "desc", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "repo:owner/repo is:open", + "sort": "created", + "order": "desc", + "page": float64(1), + "perPage": float64(30), + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "pull request search with owner and repo parameters", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "repo:test-owner/test-repo is:pr draft:false", + "sort": "updated", + "order": "asc", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "draft:false", + "owner": "test-owner", + "repo": "test-repo", + "sort": "updated", + "order": "asc", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "pull request search with only owner parameter (should ignore it)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "is:pr feature", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "feature", + "owner": "test-owner", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "pull request search with only repo parameter (should ignore it)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + expectQueryParams( + t, + map[string]string{ + "q": "is:pr review-required", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), + ), + ), + requestArgs: map[string]interface{}{ + "query": "review-required", + "repo": "test-repo", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "pull request search with minimal parameters", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetSearchIssues, + mockSearchResult, + ), + ), + requestArgs: map[string]interface{}{ + "query": "is:pr repo:owner/repo is:open", + }, + expectError: false, + expectedResult: mockSearchResult, + }, + { + name: "search pull requests fails", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetSearchIssues, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "query": "invalid:query", + }, + expectError: true, + expectedErrMsg: "failed to search pull requests", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + _, handler := SearchPullRequests(stubGetClientFn(client), translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + + // Verify results + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + + require.NoError(t, err) + + // Parse the result and get the text content if no error + textContent := getTextResult(t, result) + + // Unmarshal and verify the result + var returnedResult github.IssuesSearchResult + err = json.Unmarshal([]byte(textContent.Text), &returnedResult) + require.NoError(t, err) + assert.Equal(t, *tc.expectedResult.Total, *returnedResult.Total) + assert.Equal(t, *tc.expectedResult.IncompleteResults, *returnedResult.IncompleteResults) + assert.Len(t, returnedResult.Issues, len(tc.expectedResult.Issues)) + for i, issue := range returnedResult.Issues { + assert.Equal(t, *tc.expectedResult.Issues[i].Number, *issue.Number) + assert.Equal(t, *tc.expectedResult.Issues[i].Title, *issue.Title) + assert.Equal(t, *tc.expectedResult.Issues[i].State, *issue.State) + assert.Equal(t, *tc.expectedResult.Issues[i].HTMLURL, *issue.HTMLURL) + assert.Equal(t, *tc.expectedResult.Issues[i].User.Login, *issue.User.Login) + } + }) + } + +} + func Test_GetPullRequestFiles(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) diff --git a/pkg/github/search_utils.go b/pkg/github/search_utils.go new file mode 100644 index 000000000..6642dad8f --- /dev/null +++ b/pkg/github/search_utils.go @@ -0,0 +1,88 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + + "github.com/google/go-github/v72/github" + "github.com/mark3labs/mcp-go/mcp" +) + +func searchHandler( + ctx context.Context, + getClient GetClientFn, + request mcp.CallToolRequest, + searchType string, + errorPrefix string, +) (*mcp.CallToolResult, error) { + query, err := RequiredParam[string](request, "query") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + query = fmt.Sprintf("is:%s %s", searchType, query) + + owner, err := OptionalParam[string](request, "owner") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + repo, err := OptionalParam[string](request, "repo") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + if owner != "" && repo != "" { + query = fmt.Sprintf("repo:%s/%s %s", owner, repo, query) + } + + sort, err := OptionalParam[string](request, "sort") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + order, err := OptionalParam[string](request, "order") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + pagination, err := OptionalPaginationParams(request) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + opts := &github.SearchOptions{ + // Default to "created" if no sort is provided, as it's a common use case. + Sort: sort, + Order: order, + ListOptions: github.ListOptions{ + Page: pagination.page, + PerPage: pagination.perPage, + }, + } + + client, err := getClient(ctx) + if err != nil { + return nil, fmt.Errorf("%s: failed to get GitHub client: %w", errorPrefix, err) + } + result, resp, err := client.Search.Issues(ctx, query, opts) + if err != nil { + return nil, fmt.Errorf("%s: %w", errorPrefix, err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("%s: failed to read response body: %w", errorPrefix, err) + } + return mcp.NewToolResultError(fmt.Sprintf("%s: %s", errorPrefix, string(body))), nil + } + + r, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("%s: failed to marshal response: %w", errorPrefix, err) + } + + return mcp.NewToolResultText(string(r)), nil +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 06088a36b..76b31d477 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -73,6 +73,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG toolsets.NewServerTool(GetPullRequest(getClient, t)), toolsets.NewServerTool(ListPullRequests(getClient, t)), toolsets.NewServerTool(GetPullRequestFiles(getClient, t)), + toolsets.NewServerTool(SearchPullRequests(getClient, t)), toolsets.NewServerTool(GetPullRequestStatus(getClient, t)), toolsets.NewServerTool(GetPullRequestComments(getClient, t)), toolsets.NewServerTool(GetPullRequestReviews(getClient, t)),Fetched URL: http://github.com/github/github-mcp-server/pull/583.diff
Alternative Proxies: