From d7c32ff343b7e755e84e510bd1af6bd883171222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 09:20:18 +0000 Subject: [PATCH 01/13] Initial version of find_closing_pull_requests --- e2e/e2e_test.go | 153 ++++++++++++ .../find_closing_prs_integration_test.go | 164 +++++++++++++ pkg/github/issues.go | 217 ++++++++++++++++++ pkg/github/issues_test.go | 127 ++++++++++ pkg/github/tools.go | 1 + 5 files changed, 662 insertions(+) create mode 100644 pkg/github/find_closing_prs_integration_test.go diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 64c5729ba..80b296ae1 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -21,6 +21,7 @@ import ( gogithub "github.com/google/go-github/v73/github" mcpClient "github.com/mark3labs/mcp-go/client" "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1624,3 +1625,155 @@ func TestPullRequestReviewDeletion(t *testing.T) { require.NoError(t, err, "expected to unmarshal text content successfully") require.Len(t, noReviews, 0, "expected to find no reviews") } + +func TestFindClosingPullRequests(t *testing.T) { + t.Parallel() + + mcpClient := setupMCPClient(t, withToolsets([]string{"issues"})) + + ctx := context.Background() + + // Test with well-known GitHub repositories and issues + testCases := []struct { + name string + issues []interface{} + limit int + expectError bool + expectedResults int + expectSomeWithClosingPR bool + }{ + { + name: "Single issue test - should handle gracefully even if no closing PRs", + issues: []interface{}{"octocat/Hello-World#1"}, + limit: 5, + expectError: false, + expectedResults: 1, + }, + { + name: "Multiple issues test", + issues: []interface{}{"octocat/Hello-World#1", "github/docs#1"}, + limit: 3, + expectError: false, + expectedResults: 2, + }, + { + name: "Invalid issue format should return error", + issues: []interface{}{"invalid-format"}, + expectError: true, + }, + { + name: "Empty issues array should return error", + issues: []interface{}{}, + expectError: true, + }, + { + name: "Limit too high should return error", + issues: []interface{}{"octocat/Hello-World#1"}, + limit: 150, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Prepare the request + findClosingPRsRequest := mcp.CallToolRequest{} + findClosingPRsRequest.Params.Name = "find_closing_pull_requests" + + // Build arguments map + args := map[string]any{ + "issues": tc.issues, + } + if tc.limit > 0 { + args["limit"] = tc.limit + } + findClosingPRsRequest.Params.Arguments = args + + t.Logf("Calling find_closing_pull_requests with issues: %v", tc.issues) + resp, err := mcpClient.CallTool(ctx, findClosingPRsRequest) + + if tc.expectError { + // We expect either an error or an error response + if err != nil { + t.Logf("Expected error occurred: %v", err) + return + } + require.True(t, resp.IsError, "Expected error response") + t.Logf("Expected error in response: %+v", resp) + return + } + + require.NoError(t, err, "expected to call 'find_closing_pull_requests' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Verify we got content + require.NotEmpty(t, resp.Content, "Expected response content") + + textContent, ok := resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + t.Logf("Response: %s", textContent.Text) + + // Parse the JSON response + var response struct { + Results []struct { + Issue string `json:"issue"` + Owner string `json:"owner"` + Repo string `json:"repo"` + IssueNumber int `json:"issueNumber"` + ClosingPullRequests []struct { + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + URL string `json:"url"` + Merged bool `json:"merged"` + } `json:"closingPullRequests"` + TotalCount int `json:"totalCount"` + Error string `json:"error,omitempty"` + } `json:"results"` + } + + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err, "expected to unmarshal response successfully") + + // Verify the response structure + require.Len(t, response.Results, tc.expectedResults, "Expected specific number of results") + + // Log and verify each result + for i, result := range response.Results { + t.Logf("Result %d:", i+1) + t.Logf(" Issue: %s", result.Issue) + t.Logf(" Owner: %s, Repo: %s, Number: %d", result.Owner, result.Repo, result.IssueNumber) + t.Logf(" Total closing PRs: %d", result.TotalCount) + if result.Error != "" { + t.Logf(" Error: %s", result.Error) + } + + // Verify basic structure + assert.NotEmpty(t, result.Issue, "Issue reference should not be empty") + assert.NotEmpty(t, result.Owner, "Owner should not be empty") + assert.NotEmpty(t, result.Repo, "Repo should not be empty") + assert.Greater(t, result.IssueNumber, 0, "Issue number should be positive") + + // Log details of any closing PRs found + for j, pr := range result.ClosingPullRequests { + t.Logf(" Closing PR %d:", j+1) + t.Logf(" Number: %d", pr.Number) + t.Logf(" Title: %s", pr.Title) + t.Logf(" State: %s, Merged: %t", pr.State, pr.Merged) + t.Logf(" URL: %s", pr.URL) + + // Verify PR structure + assert.Greater(t, pr.Number, 0, "PR number should be positive") + assert.NotEmpty(t, pr.Title, "PR title should not be empty") + assert.NotEmpty(t, pr.State, "PR state should not be empty") + assert.NotEmpty(t, pr.URL, "PR URL should not be empty") + } + + // The actual count of closing PRs should match the returned array length + assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should match TotalCount") + } + }) + } +} diff --git a/pkg/github/find_closing_prs_integration_test.go b/pkg/github/find_closing_prs_integration_test.go new file mode 100644 index 000000000..8e74de0d9 --- /dev/null +++ b/pkg/github/find_closing_prs_integration_test.go @@ -0,0 +1,164 @@ +//go:build e2e + +package github + +import ( + "context" + "encoding/json" + "os" + "testing" + + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v73/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestFindClosingPullRequestsIntegration tests the FindClosingPullRequests tool with real GitHub API calls +func TestFindClosingPullRequestsIntegration(t *testing.T) { + // This test requires a GitHub token + token := os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN") + if token == "" { + t.Skip("GITHUB_MCP_SERVER_E2E_TOKEN environment variable is not set") + } + + // Create GitHub clients + httpClient := github.NewClient(nil).WithAuthToken(token).Client() + gqlClient := githubv4.NewClient(httpClient) + + getGQLClient := func(ctx context.Context) (*githubv4.Client, error) { + return gqlClient, nil + } + + // Create the tool + tool, handler := FindClosingPullRequests(getGQLClient, translations.NullTranslationHelper) + + // Test cases with known GitHub issues that were closed by PRs + testCases := []struct { + name string + issues []string + expectedResults int + expectSomeClosingPRs bool + expectSpecificIssue string + expectSpecificPRNumber int + }{ + { + name: "Single issue - VS Code well-known closed issue", + issues: []string{"microsoft/vscode#123456"}, // This is a made-up issue for testing + expectedResults: 1, + expectSomeClosingPRs: false, // We expect this to not exist or have no closing PRs + }, + { + name: "Multiple issues with mixed results", + issues: []string{"octocat/Hello-World#1", "microsoft/vscode#999999"}, + expectedResults: 2, + expectSomeClosingPRs: false, // These are likely non-existent or have no closing PRs + }, + { + name: "Issue from a popular repo - React", + issues: []string{"facebook/react#1"}, // Very first issue in React repo + expectedResults: 1, + }, + } + + ctx := context.Background() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create request arguments + args := map[string]interface{}{ + "issues": tc.issues, + "limit": 5, + } + + // Create mock request + request := mockCallToolRequest{ + arguments: args, + } + + // Call the handler + result, err := handler(ctx, request) + + if err != nil { + t.Logf("Error calling tool: %v", err) + // For integration tests, we might expect some errors for non-existent issues + // Let's check if it's a reasonable error + assert.Contains(t, err.Error(), "failed to") + return + } + + require.NotNil(t, result) + assert.False(t, result.IsError, "Expected successful result") + + // Parse the response + textContent, ok := result.Content[0].(map[string]interface{}) + if !ok { + // Try to get as text content + if len(result.Content) > 0 { + if textResult, ok := result.Content[0].(string); ok { + t.Logf("Response: %s", textResult) + + // Parse JSON response + var response struct { + Results []FindClosingPRsResult `json:"results"` + } + err := json.Unmarshal([]byte(textResult), &response) + require.NoError(t, err, "Failed to parse JSON response") + + // Verify structure + assert.Len(t, response.Results, tc.expectedResults, "Expected specific number of results") + + for i, result := range response.Results { + t.Logf("Issue %d: %s", i+1, result.Issue) + t.Logf(" Owner: %s, Repo: %s, Number: %d", result.Owner, result.Repo, result.IssueNumber) + t.Logf(" Total closing PRs: %d", result.TotalCount) + t.Logf(" Error: %s", result.Error) + + // Verify basic structure + assert.NotEmpty(t, result.Issue, "Issue reference should not be empty") + assert.NotEmpty(t, result.Owner, "Owner should not be empty") + assert.NotEmpty(t, result.Repo, "Repo should not be empty") + assert.Greater(t, result.IssueNumber, 0, "Issue number should be positive") + + // Log closing PRs if any + for j, pr := range result.ClosingPullRequests { + t.Logf(" PR %d: #%d - %s", j+1, pr.Number, pr.Title) + t.Logf(" State: %s, Merged: %t", pr.State, pr.Merged) + t.Logf(" URL: %s", pr.URL) + } + + // Check for expected specific results + if tc.expectSpecificIssue != "" && result.Issue == tc.expectSpecificIssue { + if tc.expectSpecificPRNumber > 0 { + found := false + for _, pr := range result.ClosingPullRequests { + if pr.Number == tc.expectSpecificPRNumber { + found = true + break + } + } + assert.True(t, found, "Expected to find specific PR number") + } + } + } + + return + } + } + t.Fatalf("Unexpected content type: %T", result.Content[0]) + } + + t.Logf("Response content: %+v", textContent) + }) + } +} + +// mockCallToolRequest implements the mcp.CallToolRequest interface for testing +type mockCallToolRequest struct { + arguments map[string]interface{} +} + +func (m mockCallToolRequest) GetArguments() map[string]interface{} { + return m.arguments +} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index f718c37cb..f11a71cc6 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strconv" "strings" "time" @@ -1321,3 +1322,219 @@ func AssignCodingAgentPrompt(t translations.TranslationHelperFunc) (tool mcp.Pro }, nil } } + +// IssueRef represents a parsed issue reference in owner/repo#number format +type IssueRef struct { + Owner string + Repo string + IssueNumber int + OriginalRef string +} + +// parseIssueReference parses an issue reference in the format "owner/repo#number" +func parseIssueReference(ref string) (IssueRef, error) { + // Split by # to separate repo and issue number + parts := strings.Split(ref, "#") + if len(parts) != 2 { + return IssueRef{}, fmt.Errorf("expected format: owner/repo#number, got: %s", ref) + } + + // Split by / to separate owner and repo + repoParts := strings.Split(parts[0], "/") + if len(repoParts) != 2 { + return IssueRef{}, fmt.Errorf("expected format: owner/repo#number, got: %s", ref) + } + + // Validate that owner and repo are not empty + if repoParts[0] == "" || repoParts[1] == "" { + return IssueRef{}, fmt.Errorf("owner and repo cannot be empty, got: %s", ref) + } + + // Parse issue number + issueNumber, err := strconv.Atoi(parts[1]) + if err != nil { + return IssueRef{}, fmt.Errorf("invalid issue number '%s': %v", parts[1], err) + } + + if issueNumber <= 0 { + return IssueRef{}, fmt.Errorf("issue number must be positive, got: %d", issueNumber) + } + + return IssueRef{ + Owner: repoParts[0], + Repo: repoParts[1], + IssueNumber: issueNumber, + OriginalRef: ref, + }, nil +} + +// ClosingPRNode represents a pull request that closed an issue +type ClosingPRNode struct { + Number githubv4.Int + Title githubv4.String + Body githubv4.String + State githubv4.String + URL githubv4.String + Merged githubv4.Boolean +} + +// ClosingPRsFragment represents the closedByPullRequestsReferences field +type ClosingPRsFragment struct { + TotalCount githubv4.Int + Nodes []ClosingPRNode +} + +// ClosingPullRequest represents a pull request in the response format +type ClosingPullRequest struct { + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + URL string `json:"url"` + Merged bool `json:"merged"` +} + +// FindClosingPRsResult represents the result for a single issue +type FindClosingPRsResult struct { + Issue string `json:"issue"` + Owner string `json:"owner"` + Repo string `json:"repo"` + IssueNumber int `json:"issueNumber"` + ClosingPullRequests []ClosingPullRequest `json:"closingPullRequests"` + TotalCount int `json:"totalCount"` + Error string `json:"error,omitempty"` +} + +// FindClosingPullRequests creates a tool to find pull requests that closed specific issues +func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { + return mcp.NewTool("find_closing_pull_requests", + mcp.WithDescription(t("TOOL_FIND_CLOSING_PULL_REQUESTS_DESCRIPTION", "Find pull requests that closed specific issues using closing references. Takes a list of issues in owner/repo#number format and returns the PRs that closed them.")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_FIND_CLOSING_PULL_REQUESTS_USER_TITLE", "Find closing pull requests"), + ReadOnlyHint: ToBoolPtr(true), + }), + mcp.WithArray("issues", + mcp.Required(), + mcp.Items(map[string]interface{}{ + "type": "string", + "description": "Issue reference in format owner/repo#number", + "pattern": "^[^/]+/[^/]+#[1-9][0-9]*$", + }), + mcp.Description("Array of issue references in format owner/repo#number (e.g., ['github/github#123', 'microsoft/vscode#456'])"), + ), + mcp.WithNumber("limit", + mcp.Description("Maximum number of closing PRs to return per issue (default: 10, max: 100)"), + ), + ), + func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + // Parse the issues array parameter + issuesParam, ok := request.GetArguments()["issues"].([]interface{}) + if !ok { + return mcp.NewToolResultError("issues parameter must be an array of strings"), nil + } + + if len(issuesParam) == 0 { + return mcp.NewToolResultError("issues array cannot be empty"), nil + } + + if len(issuesParam) > 50 { + return mcp.NewToolResultError("maximum 50 issues can be processed at once"), nil + } + + // Parse limit parameter + limit := 10 // default + if limitParam, exists := request.GetArguments()["limit"]; exists { + if limitFloat, ok := limitParam.(float64); ok { + limit = int(limitFloat) + if limit <= 0 || limit > 100 { + return mcp.NewToolResultError("limit must be between 1 and 100"), nil + } + } else { + return mcp.NewToolResultError("limit must be a number"), nil + } + } + + // Parse each issue reference + var issueRefs []IssueRef + for i, issueInterface := range issuesParam { + issueStr, ok := issueInterface.(string) + if !ok { + return mcp.NewToolResultError(fmt.Sprintf("issue at index %d must be a string", i)), nil + } + + ref, err := parseIssueReference(issueStr) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("invalid issue format at index %d: %v", i, err)), nil + } + issueRefs = append(issueRefs, ref) + } + + // Get GraphQL client + client, err := getGQLClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GraphQL client: %w", err) + } + + // Process each issue + var results []FindClosingPRsResult + for _, issue := range issueRefs { + result := FindClosingPRsResult{ + Issue: issue.OriginalRef, + Owner: issue.Owner, + Repo: issue.Repo, + IssueNumber: issue.IssueNumber, + } + + // Define the GraphQL query for this specific issue + var query struct { + Repository struct { + Issue struct { + ClosedByPullRequestsReferences ClosingPRsFragment `graphql:"closedByPullRequestsReferences(first: $limit)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + variables := map[string]any{ + "owner": githubv4.String(issue.Owner), + "repo": githubv4.String(issue.Repo), + "number": githubv4.Int(issue.IssueNumber), + "limit": githubv4.Int(limit), + } + + err := client.Query(ctx, &query, variables) + if err != nil { + // Don't fail the entire request for one bad issue, just record the error + result.Error = fmt.Sprintf("failed to query issue: %v", err) + results = append(results, result) + continue + } + + // Convert GraphQL response to JSON format + result.TotalCount = int(query.Repository.Issue.ClosedByPullRequestsReferences.TotalCount) + for _, node := range query.Repository.Issue.ClosedByPullRequestsReferences.Nodes { + result.ClosingPullRequests = append(result.ClosingPullRequests, ClosingPullRequest{ + Number: int(node.Number), + Title: string(node.Title), + Body: string(node.Body), + State: string(node.State), + URL: string(node.URL), + Merged: bool(node.Merged), + }) + } + + results = append(results, result) + } + + // Return results + response := map[string]interface{}{ + "results": results, + } + + responseJSON, err := json.Marshal(response) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return mcp.NewToolResultText(string(responseJSON)), nil + } +} diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 2bdb89b06..0db41d18e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -2601,3 +2601,130 @@ func Test_ReprioritizeSubIssue(t *testing.T) { }) } } + +func TestParseIssueReference(t *testing.T) { + tests := []struct { + name string + input string + expected IssueRef + expectError bool + }{ + { + name: "valid issue reference", + input: "github/copilot#123", + expected: IssueRef{ + Owner: "github", + Repo: "copilot", + IssueNumber: 123, + OriginalRef: "github/copilot#123", + }, + expectError: false, + }, + { + name: "valid issue reference with org", + input: "microsoft/vscode#456", + expected: IssueRef{ + Owner: "microsoft", + Repo: "vscode", + IssueNumber: 456, + OriginalRef: "microsoft/vscode#456", + }, + expectError: false, + }, + { + name: "missing hash", + input: "github/copilot123", + expectError: true, + }, + { + name: "missing slash", + input: "githubcopilot#123", + expectError: true, + }, + { + name: "empty owner", + input: "/copilot#123", + expectError: true, + }, + { + name: "empty repo", + input: "github/#123", + expectError: true, + }, + { + name: "invalid issue number", + input: "github/copilot#abc", + expectError: true, + }, + { + name: "zero issue number", + input: "github/copilot#0", + expectError: true, + }, + { + name: "negative issue number", + input: "github/copilot#-1", + expectError: true, + }, + { + name: "too many hash symbols", + input: "github/copilot#123#456", + expectError: true, + }, + { + name: "too many slashes", + input: "github/copilot/extra#123", + expectError: true, + }, + { + name: "empty string", + input: "", + expectError: true, + }, + { + name: "large issue number", + input: "github/copilot#999999999", + expected: IssueRef{ + Owner: "github", + Repo: "copilot", + IssueNumber: 999999999, + OriginalRef: "github/copilot#999999999", + }, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := parseIssueReference(tt.input) + + if tt.expectError { + if err == nil { + t.Errorf("expected error for input %q, but got none", tt.input) + } + return + } + + if err != nil { + t.Errorf("unexpected error for input %q: %v", tt.input, err) + return + } + + if result.Owner != tt.expected.Owner { + t.Errorf("expected owner %q, got %q", tt.expected.Owner, result.Owner) + } + + if result.Repo != tt.expected.Repo { + t.Errorf("expected repo %q, got %q", tt.expected.Repo, result.Repo) + } + + if result.IssueNumber != tt.expected.IssueNumber { + t.Errorf("expected issue number %d, got %d", tt.expected.IssueNumber, result.IssueNumber) + } + + if result.OriginalRef != tt.expected.OriginalRef { + t.Errorf("expected original ref %q, got %q", tt.expected.OriginalRef, result.OriginalRef) + } + }) + } +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 7fb1d39c0..455c436cc 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -54,6 +54,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG toolsets.NewServerTool(ListIssues(getClient, t)), toolsets.NewServerTool(GetIssueComments(getClient, t)), toolsets.NewServerTool(ListSubIssues(getClient, t)), + toolsets.NewServerTool(FindClosingPullRequests(getGQLClient, t)), ). AddWriteTools( toolsets.NewServerTool(CreateIssue(getClient, t)), From f21928a720a81a9a4204b0bd0856d77aa617de9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 10:32:32 +0000 Subject: [PATCH 02/13] Add support for structured issue queries --- .../find_closing_prs_integration_test.go | 86 +++-- pkg/github/issues.go | 305 ++++++++++-------- pkg/github/issues_test.go | 15 +- pkg/github/server.go | 37 +++ pkg/github/server_test.go | 86 +++++ 5 files changed, 352 insertions(+), 177 deletions(-) diff --git a/pkg/github/find_closing_prs_integration_test.go b/pkg/github/find_closing_prs_integration_test.go index 8e74de0d9..c0823506b 100644 --- a/pkg/github/find_closing_prs_integration_test.go +++ b/pkg/github/find_closing_prs_integration_test.go @@ -37,29 +37,44 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { // Test cases with known GitHub issues that were closed by PRs testCases := []struct { name string - issues []string + owner string + repo string + issueNumbers []int + issueReferences []string expectedResults int expectSomeClosingPRs bool expectSpecificIssue string expectSpecificPRNumber int }{ { - name: "Single issue - VS Code well-known closed issue", - issues: []string{"microsoft/vscode#123456"}, // This is a made-up issue for testing + name: "Single issue using issue_numbers - VS Code well-known closed issue", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{123456}, // This is a made-up issue for testing expectedResults: 1, expectSomeClosingPRs: false, // We expect this to not exist or have no closing PRs }, { - name: "Multiple issues with mixed results", - issues: []string{"octocat/Hello-World#1", "microsoft/vscode#999999"}, + name: "Multiple issues using issue_numbers with mixed results", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1, 999999}, expectedResults: 2, expectSomeClosingPRs: false, // These are likely non-existent or have no closing PRs }, { - name: "Issue from a popular repo - React", - issues: []string{"facebook/react#1"}, // Very first issue in React repo + name: "Issue from a popular repo using issue_numbers - React", + owner: "facebook", + repo: "react", + issueNumbers: []int{1}, // Very first issue in React repo expectedResults: 1, }, + { + name: "Cross-repository queries using issue_references", + issueReferences: []string{"octocat/Hello-World#1", "facebook/react#1"}, + expectedResults: 2, + expectSomeClosingPRs: false, // These might have closing PRs + }, } ctx := context.Background() @@ -68,8 +83,16 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Create request arguments args := map[string]interface{}{ - "issues": tc.issues, - "limit": 5, + "limit": 5, + } + + // Add appropriate parameters based on test case + if len(tc.issueNumbers) > 0 { + args["owner"] = tc.owner + args["repo"] = tc.repo + args["issue_numbers"] = tc.issueNumbers + } else if len(tc.issueReferences) > 0 { + args["issue_references"] = tc.issueReferences } // Create mock request @@ -101,7 +124,7 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { // Parse JSON response var response struct { - Results []FindClosingPRsResult `json:"results"` + Results []map[string]interface{} `json:"results"` } err := json.Unmarshal([]byte(textResult), &response) require.NoError(t, err, "Failed to parse JSON response") @@ -110,35 +133,28 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { assert.Len(t, response.Results, tc.expectedResults, "Expected specific number of results") for i, result := range response.Results { - t.Logf("Issue %d: %s", i+1, result.Issue) - t.Logf(" Owner: %s, Repo: %s, Number: %d", result.Owner, result.Repo, result.IssueNumber) - t.Logf(" Total closing PRs: %d", result.TotalCount) - t.Logf(" Error: %s", result.Error) + t.Logf("Issue %d:", i+1) + t.Logf(" Owner: %v, Repo: %v, Number: %v", result["owner"], result["repo"], result["issue_number"]) + t.Logf(" Total closing PRs: %v", result["total_count"]) - // Verify basic structure - assert.NotEmpty(t, result.Issue, "Issue reference should not be empty") - assert.NotEmpty(t, result.Owner, "Owner should not be empty") - assert.NotEmpty(t, result.Repo, "Repo should not be empty") - assert.Greater(t, result.IssueNumber, 0, "Issue number should be positive") - - // Log closing PRs if any - for j, pr := range result.ClosingPullRequests { - t.Logf(" PR %d: #%d - %s", j+1, pr.Number, pr.Title) - t.Logf(" State: %s, Merged: %t", pr.State, pr.Merged) - t.Logf(" URL: %s", pr.URL) + if errorMsg, hasError := result["error"]; hasError { + t.Logf(" Error: %v", errorMsg) } - // Check for expected specific results - if tc.expectSpecificIssue != "" && result.Issue == tc.expectSpecificIssue { - if tc.expectSpecificPRNumber > 0 { - found := false - for _, pr := range result.ClosingPullRequests { - if pr.Number == tc.expectSpecificPRNumber { - found = true - break - } + // Verify basic structure + assert.NotEmpty(t, result["owner"], "Owner should not be empty") + assert.NotEmpty(t, result["repo"], "Repo should not be empty") + assert.NotNil(t, result["issue_number"], "Issue number should not be nil") + + // Check closing PRs if any + if closingPRs, ok := result["closing_pull_requests"].([]interface{}); ok { + t.Logf(" Found %d closing PRs", len(closingPRs)) + for j, pr := range closingPRs { + if prMap, ok := pr.(map[string]interface{}); ok { + t.Logf(" PR %d: #%v - %v", j+1, prMap["number"], prMap["title"]) + t.Logf(" State: %v, Merged: %v", prMap["state"], prMap["merged"]) + t.Logf(" URL: %v", prMap["url"]) } - assert.True(t, found, "Expected to find specific PR number") } } } diff --git a/pkg/github/issues.go b/pkg/github/issues.go index f11a71cc6..216a9ec07 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1323,51 +1323,6 @@ func AssignCodingAgentPrompt(t translations.TranslationHelperFunc) (tool mcp.Pro } } -// IssueRef represents a parsed issue reference in owner/repo#number format -type IssueRef struct { - Owner string - Repo string - IssueNumber int - OriginalRef string -} - -// parseIssueReference parses an issue reference in the format "owner/repo#number" -func parseIssueReference(ref string) (IssueRef, error) { - // Split by # to separate repo and issue number - parts := strings.Split(ref, "#") - if len(parts) != 2 { - return IssueRef{}, fmt.Errorf("expected format: owner/repo#number, got: %s", ref) - } - - // Split by / to separate owner and repo - repoParts := strings.Split(parts[0], "/") - if len(repoParts) != 2 { - return IssueRef{}, fmt.Errorf("expected format: owner/repo#number, got: %s", ref) - } - - // Validate that owner and repo are not empty - if repoParts[0] == "" || repoParts[1] == "" { - return IssueRef{}, fmt.Errorf("owner and repo cannot be empty, got: %s", ref) - } - - // Parse issue number - issueNumber, err := strconv.Atoi(parts[1]) - if err != nil { - return IssueRef{}, fmt.Errorf("invalid issue number '%s': %v", parts[1], err) - } - - if issueNumber <= 0 { - return IssueRef{}, fmt.Errorf("issue number must be positive, got: %d", issueNumber) - } - - return IssueRef{ - Owner: repoParts[0], - Repo: repoParts[1], - IssueNumber: issueNumber, - OriginalRef: ref, - }, nil -} - // ClosingPRNode represents a pull request that closed an issue type ClosingPRNode struct { Number githubv4.Int @@ -1394,53 +1349,41 @@ type ClosingPullRequest struct { Merged bool `json:"merged"` } -// FindClosingPRsResult represents the result for a single issue -type FindClosingPRsResult struct { - Issue string `json:"issue"` - Owner string `json:"owner"` - Repo string `json:"repo"` - IssueNumber int `json:"issueNumber"` - ClosingPullRequests []ClosingPullRequest `json:"closingPullRequests"` - TotalCount int `json:"totalCount"` - Error string `json:"error,omitempty"` -} - // FindClosingPullRequests creates a tool to find pull requests that closed specific issues func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("find_closing_pull_requests", - mcp.WithDescription(t("TOOL_FIND_CLOSING_PULL_REQUESTS_DESCRIPTION", "Find pull requests that closed specific issues using closing references. Takes a list of issues in owner/repo#number format and returns the PRs that closed them.")), + mcp.WithDescription(t("TOOL_FIND_CLOSING_PULL_REQUESTS_DESCRIPTION", "Find pull requests that closed specific issues using closing references. Supports both single repository queries (owner/repo + issue_numbers array) and cross-repository queries (issue_references array).")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_FIND_CLOSING_PULL_REQUESTS_USER_TITLE", "Find closing pull requests"), ReadOnlyHint: ToBoolPtr(true), }), - mcp.WithArray("issues", - mcp.Required(), - mcp.Items(map[string]interface{}{ - "type": "string", - "description": "Issue reference in format owner/repo#number", - "pattern": "^[^/]+/[^/]+#[1-9][0-9]*$", - }), - mcp.Description("Array of issue references in format owner/repo#number (e.g., ['github/github#123', 'microsoft/vscode#456'])"), + mcp.WithString("owner", + mcp.Description("The owner of the repository (required if using issue_numbers)"), + ), + mcp.WithString("repo", + mcp.Description("The name of the repository (required if using issue_numbers)"), + ), + mcp.WithArray("issue_numbers", + mcp.Description("Array of issue numbers within the specified repository"), + mcp.Items( + map[string]any{ + "type": "number", + }, + ), + ), + mcp.WithArray("issue_references", + mcp.Description("Array of issue references in format 'owner/repo#number' for cross-repository queries"), + mcp.Items( + map[string]any{ + "type": "string", + }, + ), ), mcp.WithNumber("limit", mcp.Description("Maximum number of closing PRs to return per issue (default: 10, max: 100)"), ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // Parse the issues array parameter - issuesParam, ok := request.GetArguments()["issues"].([]interface{}) - if !ok { - return mcp.NewToolResultError("issues parameter must be an array of strings"), nil - } - - if len(issuesParam) == 0 { - return mcp.NewToolResultError("issues array cannot be empty"), nil - } - - if len(issuesParam) > 50 { - return mcp.NewToolResultError("maximum 50 issues can be processed at once"), nil - } - // Parse limit parameter limit := 10 // default if limitParam, exists := request.GetArguments()["limit"]; exists { @@ -1454,19 +1397,21 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } } - // Parse each issue reference - var issueRefs []IssueRef - for i, issueInterface := range issuesParam { - issueStr, ok := issueInterface.(string) - if !ok { - return mcp.NewToolResultError(fmt.Sprintf("issue at index %d must be a string", i)), nil - } + // Get optional parameters + owner, _ := OptionalParam[string](request, "owner") + repo, _ := OptionalParam[string](request, "repo") + issueNumbers, _ := OptionalIntArrayParam(request, "issue_numbers") + issueReferences, _ := OptionalStringArrayParam(request, "issue_references") - ref, err := parseIssueReference(issueStr) - if err != nil { - return mcp.NewToolResultError(fmt.Sprintf("invalid issue format at index %d: %v", i, err)), nil - } - issueRefs = append(issueRefs, ref) + // Validate input combinations + if len(issueNumbers) == 0 && len(issueReferences) == 0 { + return mcp.NewToolResultError("either issue_numbers or issue_references must be provided"), nil + } + if len(issueNumbers) > 0 && len(issueReferences) > 0 { + return mcp.NewToolResultError("provide either issue_numbers OR issue_references, not both"), nil + } + if len(issueNumbers) > 0 && (owner == "" || repo == "") { + return mcp.NewToolResultError("owner and repo are required when using issue_numbers"), nil } // Get GraphQL client @@ -1475,53 +1420,45 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla return nil, fmt.Errorf("failed to get GraphQL client: %w", err) } - // Process each issue - var results []FindClosingPRsResult - for _, issue := range issueRefs { - result := FindClosingPRsResult{ - Issue: issue.OriginalRef, - Owner: issue.Owner, - Repo: issue.Repo, - IssueNumber: issue.IssueNumber, - } + var queries []IssueQuery - // Define the GraphQL query for this specific issue - var query struct { - Repository struct { - Issue struct { - ClosedByPullRequestsReferences ClosingPRsFragment `graphql:"closedByPullRequestsReferences(first: $limit)"` - } `graphql:"issue(number: $number)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + // Build queries based on input type + if len(issueNumbers) > 0 { + // Single repository, multiple issue numbers + for _, issueNum := range issueNumbers { + queries = append(queries, IssueQuery{ + Owner: owner, + Repo: repo, + IssueNumber: issueNum, + }) } - - variables := map[string]any{ - "owner": githubv4.String(issue.Owner), - "repo": githubv4.String(issue.Repo), - "number": githubv4.Int(issue.IssueNumber), - "limit": githubv4.Int(limit), + } else { + // Multiple issue references + for _, ref := range issueReferences { + parsed, err := parseIssueReference(ref) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("invalid issue reference '%s': %s", ref, err.Error())), nil + } + queries = append(queries, parsed) } + } - err := client.Query(ctx, &query, variables) + // Process each issue query + var results []map[string]interface{} + for _, query := range queries { + result, err := queryClosingPRsForIssue(ctx, client, query.Owner, query.Repo, query.IssueNumber, limit) if err != nil { - // Don't fail the entire request for one bad issue, just record the error - result.Error = fmt.Sprintf("failed to query issue: %v", err) - results = append(results, result) - continue - } - - // Convert GraphQL response to JSON format - result.TotalCount = int(query.Repository.Issue.ClosedByPullRequestsReferences.TotalCount) - for _, node := range query.Repository.Issue.ClosedByPullRequestsReferences.Nodes { - result.ClosingPullRequests = append(result.ClosingPullRequests, ClosingPullRequest{ - Number: int(node.Number), - Title: string(node.Title), - Body: string(node.Body), - State: string(node.State), - URL: string(node.URL), - Merged: bool(node.Merged), + // Add error result for this issue + results = append(results, map[string]interface{}{ + "owner": query.Owner, + "repo": query.Repo, + "issue_number": query.IssueNumber, + "error": err.Error(), + "total_count": 0, + "closing_pull_requests": []ClosingPullRequest{}, }) + continue } - results = append(results, result) } @@ -1538,3 +1475,109 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla return mcp.NewToolResultText(string(responseJSON)), nil } } + +// IssueQuery represents a query for a specific issue +type IssueQuery struct { + Owner string + Repo string + IssueNumber int +} + +// parseIssueReference parses an issue reference in the format "owner/repo#number" +func parseIssueReference(ref string) (IssueQuery, error) { + if ref == "" { + return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") + } + + // Find the '#' separator + hashIndex := strings.LastIndex(ref, "#") + if hashIndex == -1 || hashIndex == len(ref)-1 { + return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") + } + + // Split the repo part and issue number + repoPart := ref[:hashIndex] + issueNumPart := ref[hashIndex+1:] + + // Check for multiple hash symbols (invalid case) + if strings.Count(ref, "#") > 1 { + return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") + } + + // Parse issue number + issueNum, err := strconv.Atoi(issueNumPart) + if err != nil { + return IssueQuery{}, fmt.Errorf("invalid issue number: %s", issueNumPart) + } + + // Check for negative or zero issue numbers (GitHub issue numbers are positive) + if issueNum <= 0 { + return IssueQuery{}, fmt.Errorf("invalid issue number: %s", issueNumPart) + } + + // Find the '/' separator in repo part + slashIndex := strings.Index(repoPart, "/") + if slashIndex == -1 || slashIndex == 0 || slashIndex == len(repoPart)-1 { + return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") + } + + // Check for multiple slashes in repo part (invalid case like "owner/repo/extra") + if strings.Count(repoPart, "/") > 1 { + return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") + } + + owner := repoPart[:slashIndex] + repo := repoPart[slashIndex+1:] + + return IssueQuery{ + Owner: owner, + Repo: repo, + IssueNumber: issueNum, + }, nil +} + +// queryClosingPRsForIssue queries closing PRs for a single issue +func queryClosingPRsForIssue(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber, limit int) (map[string]interface{}, error) { + // Define the GraphQL query for this specific issue + var query struct { + Repository struct { + Issue struct { + ClosedByPullRequestsReferences ClosingPRsFragment `graphql:"closedByPullRequestsReferences(first: $limit)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + variables := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "number": githubv4.Int(issueNumber), + "limit": githubv4.Int(limit), + } + + err := client.Query(ctx, &query, variables) + if err != nil { + return nil, fmt.Errorf("failed to query issue: %w", err) + } + + // Convert GraphQL response to JSON format + var closingPullRequests []ClosingPullRequest + for _, node := range query.Repository.Issue.ClosedByPullRequestsReferences.Nodes { + closingPullRequests = append(closingPullRequests, ClosingPullRequest{ + Number: int(node.Number), + Title: string(node.Title), + Body: string(node.Body), + State: string(node.State), + URL: string(node.URL), + Merged: bool(node.Merged), + }) + } + + // Return results in format consistent with other tools + return map[string]interface{}{ + "owner": owner, + "repo": repo, + "issue_number": issueNumber, + "total_count": int(query.Repository.Issue.ClosedByPullRequestsReferences.TotalCount), + "closing_pull_requests": closingPullRequests, + }, nil +} diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 0db41d18e..0882de083 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -2606,28 +2606,26 @@ func TestParseIssueReference(t *testing.T) { tests := []struct { name string input string - expected IssueRef + expected IssueQuery expectError bool }{ { name: "valid issue reference", input: "github/copilot#123", - expected: IssueRef{ + expected: IssueQuery{ Owner: "github", Repo: "copilot", IssueNumber: 123, - OriginalRef: "github/copilot#123", }, expectError: false, }, { name: "valid issue reference with org", input: "microsoft/vscode#456", - expected: IssueRef{ + expected: IssueQuery{ Owner: "microsoft", Repo: "vscode", IssueNumber: 456, - OriginalRef: "microsoft/vscode#456", }, expectError: false, }, @@ -2684,11 +2682,10 @@ func TestParseIssueReference(t *testing.T) { { name: "large issue number", input: "github/copilot#999999999", - expected: IssueRef{ + expected: IssueQuery{ Owner: "github", Repo: "copilot", IssueNumber: 999999999, - OriginalRef: "github/copilot#999999999", }, expectError: false, }, @@ -2721,10 +2718,6 @@ func TestParseIssueReference(t *testing.T) { if result.IssueNumber != tt.expected.IssueNumber { t.Errorf("expected issue number %d, got %d", tt.expected.IssueNumber, result.IssueNumber) } - - if result.OriginalRef != tt.expected.OriginalRef { - t.Errorf("expected original ref %q, got %q", tt.expected.OriginalRef, result.OriginalRef) - } }) } } diff --git a/pkg/github/server.go b/pkg/github/server.go index 193336b75..933bdb914 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -174,6 +174,43 @@ func OptionalStringArrayParam(r mcp.CallToolRequest, p string) ([]string, error) } } +// OptionalIntArrayParam is a helper function that can be used to fetch a requested parameter from the request. +// It does the following checks: +// 1. Checks if the parameter is present in the request, if not, it returns its zero-value +// 2. If it is present, iterates the elements and checks each is a number that can be converted to int +func OptionalIntArrayParam(r mcp.CallToolRequest, p string) ([]int, error) { + // Check if the parameter is present in the request + if _, ok := r.GetArguments()[p]; !ok { + return []int{}, nil + } + + switch v := r.GetArguments()[p].(type) { + case nil: + return []int{}, nil + case []int: + return v, nil + case []any: + intSlice := make([]int, len(v)) + for i, v := range v { + switch num := v.(type) { + case float64: + intSlice[i] = int(num) + case int: + intSlice[i] = num + case int32: + intSlice[i] = int(num) + case int64: + intSlice[i] = int(num) + default: + return []int{}, fmt.Errorf("parameter %s is not of type number, is %T", p, v) + } + } + return intSlice, nil + default: + return []int{}, fmt.Errorf("parameter %s could not be coerced to []int, is %T", p, r.GetArguments()[p]) + } +} + // 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 { diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 7f8f29c0d..047463650 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -478,6 +478,92 @@ func TestOptionalStringArrayParam(t *testing.T) { } } +func TestOptionalIntArrayParam(t *testing.T) { + tests := []struct { + name string + params map[string]interface{} + paramName string + expected []int + expectError bool + }{ + { + name: "parameter not in request", + params: map[string]any{}, + paramName: "numbers", + expected: []int{}, + expectError: false, + }, + { + name: "valid any array parameter with float64", + params: map[string]any{ + "numbers": []any{float64(1), float64(2), float64(3)}, + }, + paramName: "numbers", + expected: []int{1, 2, 3}, + expectError: false, + }, + { + name: "valid int array parameter", + params: map[string]any{ + "numbers": []int{1, 2, 3}, + }, + paramName: "numbers", + expected: []int{1, 2, 3}, + expectError: false, + }, + { + name: "mixed numeric types", + params: map[string]any{ + "numbers": []any{float64(1), int(2), int32(3), int64(4)}, + }, + paramName: "numbers", + expected: []int{1, 2, 3, 4}, + expectError: false, + }, + { + name: "invalid type in array", + params: map[string]any{ + "numbers": []any{float64(1), "not a number"}, + }, + paramName: "numbers", + expected: []int{}, + expectError: true, + }, + { + name: "nil value", + params: map[string]any{ + "numbers": nil, + }, + paramName: "numbers", + expected: []int{}, + expectError: false, + }, + { + name: "wrong parameter type", + params: map[string]any{ + "numbers": "not an array", + }, + paramName: "numbers", + expected: []int{}, + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + request := createMCPRequest(tc.params) + result, err := OptionalIntArrayParam(request, tc.paramName) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expected, result) + } + }) + } +} + func TestOptionalPaginationParams(t *testing.T) { tests := []struct { name string From bea8311f4a3233550625e718639405d2e40bfd5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 10:33:54 +0000 Subject: [PATCH 03/13] Support only finding closing PRs within one repo --- e2e/e2e_test.go | 41 ++--- .../find_closing_prs_integration_test.go | 21 +-- pkg/github/issues.go | 141 ++++-------------- pkg/github/issues_test.go | 120 --------------- 4 files changed, 55 insertions(+), 268 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 80b296ae1..ae523f517 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1636,7 +1636,9 @@ func TestFindClosingPullRequests(t *testing.T) { // Test with well-known GitHub repositories and issues testCases := []struct { name string - issues []interface{} + owner string + repo string + issueNumbers []int limit int expectError bool expectedResults int @@ -1644,33 +1646,36 @@ func TestFindClosingPullRequests(t *testing.T) { }{ { name: "Single issue test - should handle gracefully even if no closing PRs", - issues: []interface{}{"octocat/Hello-World#1"}, + owner: "octocat", + repo: "Hello-World", + issueNumbers: []int{1}, limit: 5, expectError: false, expectedResults: 1, }, { name: "Multiple issues test", - issues: []interface{}{"octocat/Hello-World#1", "github/docs#1"}, + owner: "github", + repo: "docs", + issueNumbers: []int{1, 2}, limit: 3, expectError: false, expectedResults: 2, }, { - name: "Invalid issue format should return error", - issues: []interface{}{"invalid-format"}, - expectError: true, + name: "Empty issue_numbers array should return error", + owner: "octocat", + repo: "Hello-World", + issueNumbers: []int{}, + expectError: true, }, { - name: "Empty issues array should return error", - issues: []interface{}{}, - expectError: true, - }, - { - name: "Limit too high should return error", - issues: []interface{}{"octocat/Hello-World#1"}, - limit: 150, - expectError: true, + name: "Limit too high should return error", + owner: "octocat", + repo: "Hello-World", + issueNumbers: []int{1}, + limit: 150, + expectError: true, }, } @@ -1682,14 +1687,16 @@ func TestFindClosingPullRequests(t *testing.T) { // Build arguments map args := map[string]any{ - "issues": tc.issues, + "owner": tc.owner, + "repo": tc.repo, + "issue_numbers": tc.issueNumbers, } if tc.limit > 0 { args["limit"] = tc.limit } findClosingPRsRequest.Params.Arguments = args - t.Logf("Calling find_closing_pull_requests with issues: %v", tc.issues) + t.Logf("Calling find_closing_pull_requests with owner: %s, repo: %s, issue_numbers: %v", tc.owner, tc.repo, tc.issueNumbers) resp, err := mcpClient.CallTool(ctx, findClosingPRsRequest) if tc.expectError { diff --git a/pkg/github/find_closing_prs_integration_test.go b/pkg/github/find_closing_prs_integration_test.go index c0823506b..eb5c6b015 100644 --- a/pkg/github/find_closing_prs_integration_test.go +++ b/pkg/github/find_closing_prs_integration_test.go @@ -40,7 +40,6 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { owner string repo string issueNumbers []int - issueReferences []string expectedResults int expectSomeClosingPRs bool expectSpecificIssue string @@ -69,12 +68,6 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { issueNumbers: []int{1}, // Very first issue in React repo expectedResults: 1, }, - { - name: "Cross-repository queries using issue_references", - issueReferences: []string{"octocat/Hello-World#1", "facebook/react#1"}, - expectedResults: 2, - expectSomeClosingPRs: false, // These might have closing PRs - }, } ctx := context.Background() @@ -83,16 +76,10 @@ func TestFindClosingPullRequestsIntegration(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Create request arguments args := map[string]interface{}{ - "limit": 5, - } - - // Add appropriate parameters based on test case - if len(tc.issueNumbers) > 0 { - args["owner"] = tc.owner - args["repo"] = tc.repo - args["issue_numbers"] = tc.issueNumbers - } else if len(tc.issueReferences) > 0 { - args["issue_references"] = tc.issueReferences + "limit": 5, + "owner": tc.owner, + "repo": tc.repo, + "issue_numbers": tc.issueNumbers, } // Create mock request diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 216a9ec07..3cc7862b9 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "net/http" - "strconv" "strings" "time" @@ -1352,33 +1351,28 @@ type ClosingPullRequest struct { // FindClosingPullRequests creates a tool to find pull requests that closed specific issues func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("find_closing_pull_requests", - mcp.WithDescription(t("TOOL_FIND_CLOSING_PULL_REQUESTS_DESCRIPTION", "Find pull requests that closed specific issues using closing references. Supports both single repository queries (owner/repo + issue_numbers array) and cross-repository queries (issue_references array).")), + mcp.WithDescription(t("TOOL_FIND_CLOSING_PULL_REQUESTS_DESCRIPTION", "Find pull requests that closed specific issues using closing references within a repository.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_FIND_CLOSING_PULL_REQUESTS_USER_TITLE", "Find closing pull requests"), ReadOnlyHint: ToBoolPtr(true), }), mcp.WithString("owner", - mcp.Description("The owner of the repository (required if using issue_numbers)"), + mcp.Description("The owner of the repository"), + mcp.Required(), ), mcp.WithString("repo", - mcp.Description("The name of the repository (required if using issue_numbers)"), + mcp.Description("The name of the repository"), + mcp.Required(), ), mcp.WithArray("issue_numbers", mcp.Description("Array of issue numbers within the specified repository"), + mcp.Required(), mcp.Items( map[string]any{ "type": "number", }, ), ), - mcp.WithArray("issue_references", - mcp.Description("Array of issue references in format 'owner/repo#number' for cross-repository queries"), - mcp.Items( - map[string]any{ - "type": "string", - }, - ), - ), mcp.WithNumber("limit", mcp.Description("Maximum number of closing PRs to return per issue (default: 10, max: 100)"), ), @@ -1397,21 +1391,23 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } } - // Get optional parameters - owner, _ := OptionalParam[string](request, "owner") - repo, _ := OptionalParam[string](request, "repo") - issueNumbers, _ := OptionalIntArrayParam(request, "issue_numbers") - issueReferences, _ := OptionalStringArrayParam(request, "issue_references") - - // Validate input combinations - if len(issueNumbers) == 0 && len(issueReferences) == 0 { - return mcp.NewToolResultError("either issue_numbers or issue_references must be provided"), nil + // Get required parameters + owner, err := RequiredParam[string](request, "owner") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("owner parameter error: %s", err.Error())), nil + } + repo, err := RequiredParam[string](request, "repo") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("repo parameter error: %s", err.Error())), nil } - if len(issueNumbers) > 0 && len(issueReferences) > 0 { - return mcp.NewToolResultError("provide either issue_numbers OR issue_references, not both"), nil + issueNumbers, err := OptionalIntArrayParam(request, "issue_numbers") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("issue_numbers parameter error: %s", err.Error())), nil } - if len(issueNumbers) > 0 && (owner == "" || repo == "") { - return mcp.NewToolResultError("owner and repo are required when using issue_numbers"), nil + + // Validate that issue_numbers is not empty + if len(issueNumbers) == 0 { + return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty"), nil } // Get GraphQL client @@ -1420,39 +1416,16 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla return nil, fmt.Errorf("failed to get GraphQL client: %w", err) } - var queries []IssueQuery - - // Build queries based on input type - if len(issueNumbers) > 0 { - // Single repository, multiple issue numbers - for _, issueNum := range issueNumbers { - queries = append(queries, IssueQuery{ - Owner: owner, - Repo: repo, - IssueNumber: issueNum, - }) - } - } else { - // Multiple issue references - for _, ref := range issueReferences { - parsed, err := parseIssueReference(ref) - if err != nil { - return mcp.NewToolResultError(fmt.Sprintf("invalid issue reference '%s': %s", ref, err.Error())), nil - } - queries = append(queries, parsed) - } - } - - // Process each issue query + // Process each issue number var results []map[string]interface{} - for _, query := range queries { - result, err := queryClosingPRsForIssue(ctx, client, query.Owner, query.Repo, query.IssueNumber, limit) + for _, issueNum := range issueNumbers { + result, err := queryClosingPRsForIssue(ctx, client, owner, repo, issueNum, limit) if err != nil { // Add error result for this issue results = append(results, map[string]interface{}{ - "owner": query.Owner, - "repo": query.Repo, - "issue_number": query.IssueNumber, + "owner": owner, + "repo": repo, + "issue_number": issueNum, "error": err.Error(), "total_count": 0, "closing_pull_requests": []ClosingPullRequest{}, @@ -1476,66 +1449,6 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } } -// IssueQuery represents a query for a specific issue -type IssueQuery struct { - Owner string - Repo string - IssueNumber int -} - -// parseIssueReference parses an issue reference in the format "owner/repo#number" -func parseIssueReference(ref string) (IssueQuery, error) { - if ref == "" { - return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") - } - - // Find the '#' separator - hashIndex := strings.LastIndex(ref, "#") - if hashIndex == -1 || hashIndex == len(ref)-1 { - return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") - } - - // Split the repo part and issue number - repoPart := ref[:hashIndex] - issueNumPart := ref[hashIndex+1:] - - // Check for multiple hash symbols (invalid case) - if strings.Count(ref, "#") > 1 { - return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") - } - - // Parse issue number - issueNum, err := strconv.Atoi(issueNumPart) - if err != nil { - return IssueQuery{}, fmt.Errorf("invalid issue number: %s", issueNumPart) - } - - // Check for negative or zero issue numbers (GitHub issue numbers are positive) - if issueNum <= 0 { - return IssueQuery{}, fmt.Errorf("invalid issue number: %s", issueNumPart) - } - - // Find the '/' separator in repo part - slashIndex := strings.Index(repoPart, "/") - if slashIndex == -1 || slashIndex == 0 || slashIndex == len(repoPart)-1 { - return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") - } - - // Check for multiple slashes in repo part (invalid case like "owner/repo/extra") - if strings.Count(repoPart, "/") > 1 { - return IssueQuery{}, fmt.Errorf("invalid format, expected 'owner/repo#number'") - } - - owner := repoPart[:slashIndex] - repo := repoPart[slashIndex+1:] - - return IssueQuery{ - Owner: owner, - Repo: repo, - IssueNumber: issueNum, - }, nil -} - // queryClosingPRsForIssue queries closing PRs for a single issue func queryClosingPRsForIssue(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber, limit int) (map[string]interface{}, error) { // Define the GraphQL query for this specific issue diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 0882de083..2bdb89b06 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -2601,123 +2601,3 @@ func Test_ReprioritizeSubIssue(t *testing.T) { }) } } - -func TestParseIssueReference(t *testing.T) { - tests := []struct { - name string - input string - expected IssueQuery - expectError bool - }{ - { - name: "valid issue reference", - input: "github/copilot#123", - expected: IssueQuery{ - Owner: "github", - Repo: "copilot", - IssueNumber: 123, - }, - expectError: false, - }, - { - name: "valid issue reference with org", - input: "microsoft/vscode#456", - expected: IssueQuery{ - Owner: "microsoft", - Repo: "vscode", - IssueNumber: 456, - }, - expectError: false, - }, - { - name: "missing hash", - input: "github/copilot123", - expectError: true, - }, - { - name: "missing slash", - input: "githubcopilot#123", - expectError: true, - }, - { - name: "empty owner", - input: "/copilot#123", - expectError: true, - }, - { - name: "empty repo", - input: "github/#123", - expectError: true, - }, - { - name: "invalid issue number", - input: "github/copilot#abc", - expectError: true, - }, - { - name: "zero issue number", - input: "github/copilot#0", - expectError: true, - }, - { - name: "negative issue number", - input: "github/copilot#-1", - expectError: true, - }, - { - name: "too many hash symbols", - input: "github/copilot#123#456", - expectError: true, - }, - { - name: "too many slashes", - input: "github/copilot/extra#123", - expectError: true, - }, - { - name: "empty string", - input: "", - expectError: true, - }, - { - name: "large issue number", - input: "github/copilot#999999999", - expected: IssueQuery{ - Owner: "github", - Repo: "copilot", - IssueNumber: 999999999, - }, - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := parseIssueReference(tt.input) - - if tt.expectError { - if err == nil { - t.Errorf("expected error for input %q, but got none", tt.input) - } - return - } - - if err != nil { - t.Errorf("unexpected error for input %q: %v", tt.input, err) - return - } - - if result.Owner != tt.expected.Owner { - t.Errorf("expected owner %q, got %q", tt.expected.Owner, result.Owner) - } - - if result.Repo != tt.expected.Repo { - t.Errorf("expected repo %q, got %q", tt.expected.Repo, result.Repo) - } - - if result.IssueNumber != tt.expected.IssueNumber { - t.Errorf("expected issue number %d, got %d", tt.expected.IssueNumber, result.IssueNumber) - } - }) - } -} From bce7b38d2012d769da91913e8b85a0e1743c8552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 10:53:06 +0000 Subject: [PATCH 04/13] Fix formatting --- pkg/github/discussions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 9458dfce0..d1dcf064a 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -484,7 +484,7 @@ func Test_GetDiscussion(t *testing.T) { assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"}) // 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,title,body,createdAt,url,category{name}}}}" + qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,title,body,createdAt,url,category{name}}}}" vars := map[string]interface{}{ "owner": "owner", From 66ce1a63f13690eae88df4f2cc7cd6e6d2431468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 14:26:00 +0000 Subject: [PATCH 05/13] Add support for API options --- e2e/e2e_test.go | 467 +++++++++++++++++++++++++++++++++++++++++++ pkg/github/issues.go | 157 ++++++++++++++- 2 files changed, 614 insertions(+), 10 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index ae523f517..478d4678f 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1784,3 +1784,470 @@ func TestFindClosingPullRequests(t *testing.T) { }) } } + +// TestFindClosingPullRequestsGraphQLParameters tests the enhanced GraphQL parameters +func TestFindClosingPullRequestsGraphQLParameters(t *testing.T) { + t.Parallel() + + mcpClient := setupMCPClient(t, withToolsets([]string{"issues"})) + ctx := context.Background() + + t.Run("Boolean Parameters", func(t *testing.T) { + // Test cases for boolean parameters + booleanTestCases := []struct { + name string + owner string + repo string + issueNumbers []int + includeClosedPrs *bool + orderByState *bool + userLinkedOnly *bool + expectError bool + description string + }{ + { + name: "includeClosedPrs=true - should include closed/merged PRs", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + includeClosedPrs: boolPtr(true), + description: "Test includeClosedPrs parameter with popular repository", + }, + { + name: "includeClosedPrs=false - should exclude closed/merged PRs", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{2}, + includeClosedPrs: boolPtr(false), + description: "Test includeClosedPrs=false parameter", + }, + { + name: "orderByState=true - should order results by PR state", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1, 2}, // Use low numbers for older issues + orderByState: boolPtr(true), + description: "Test orderByState parameter with larger repository", + }, + { + name: "userLinkedOnly=true - should return only manually linked PRs", + owner: "facebook", + repo: "react", + issueNumbers: []int{1}, // First issue in React repo + userLinkedOnly: boolPtr(true), + description: "Test userLinkedOnly parameter", + }, + { + name: "Combined boolean parameters", + owner: "facebook", + repo: "react", + issueNumbers: []int{1, 2}, + includeClosedPrs: boolPtr(true), + orderByState: boolPtr(true), + userLinkedOnly: boolPtr(false), + description: "Test multiple boolean parameters together", + }, + } + + for _, tc := range booleanTestCases { + t.Run(tc.name, func(t *testing.T) { + // Build arguments map + args := map[string]any{ + "owner": tc.owner, + "repo": tc.repo, + "issue_numbers": tc.issueNumbers, + "limit": 5, // Keep limit reasonable + } + + // Add boolean parameters if specified + if tc.includeClosedPrs != nil { + args["includeClosedPrs"] = *tc.includeClosedPrs + } + if tc.orderByState != nil { + args["orderByState"] = *tc.orderByState + } + if tc.userLinkedOnly != nil { + args["userLinkedOnly"] = *tc.userLinkedOnly + } + + // Create request + request := mcp.CallToolRequest{} + request.Params.Name = "find_closing_pull_requests" + request.Params.Arguments = args + + t.Logf("Testing %s: %s", tc.name, tc.description) + t.Logf("Arguments: %+v", args) + + // Call the tool + resp, err := mcpClient.CallTool(ctx, request) + + if tc.expectError { + if err != nil { + t.Logf("Expected error occurred: %v", err) + return + } + require.True(t, resp.IsError, "Expected error response") + return + } + + require.NoError(t, err, "expected successful tool call") + require.False(t, resp.IsError, fmt.Sprintf("expected non-error response: %+v", resp)) + require.NotEmpty(t, resp.Content, "Expected response content") + + // Parse response + textContent, ok := resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected TextContent") + + var response struct { + Results []struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + IssueNumber int `json:"issue_number"` + TotalCount int `json:"total_count"` + ClosingPullRequests []struct { + Number int `json:"number"` + Title string `json:"title"` + State string `json:"state"` + Merged bool `json:"merged"` + URL string `json:"url"` + } `json:"closing_pull_requests"` + Error string `json:"error,omitempty"` + } `json:"results"` + } + + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err, "expected successful JSON parsing") + + // Verify response structure + require.NotEmpty(t, response.Results, "Expected at least one result") + + for i, result := range response.Results { + t.Logf("Result %d: Owner=%s, Repo=%s, Issue=%d, TotalCount=%d", + i+1, result.Owner, result.Repo, result.IssueNumber, result.TotalCount) + + // Log PRs found + for j, pr := range result.ClosingPullRequests { + t.Logf(" PR %d: #%d - %s (State: %s, Merged: %t)", + j+1, pr.Number, pr.Title, pr.State, pr.Merged) + } + + // Basic validations + assert.Equal(t, tc.owner, result.Owner, "Owner should match request") + assert.Equal(t, tc.repo, result.Repo, "Repo should match request") + assert.Contains(t, tc.issueNumbers, result.IssueNumber, "Issue number should be in request") + assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "TotalCount should match array length") + + // Parameter-specific validations + if tc.includeClosedPrs != nil && *tc.includeClosedPrs == false { + // When includeClosedPrs=false, should not include closed/merged PRs + for _, pr := range result.ClosingPullRequests { + assert.NotEqual(t, "CLOSED", pr.State, "Should not include closed PRs when includeClosedPrs=false") + if pr.State == "MERGED" { + assert.False(t, pr.Merged, "Should not include merged PRs when includeClosedPrs=false") + } + } + } + + if tc.orderByState != nil && *tc.orderByState == true && len(result.ClosingPullRequests) > 1 { + // When orderByState=true, verify some ordering (exact ordering depends on GitHub's implementation) + t.Logf("OrderByState=true: PRs should be ordered by state") + // Note: We can't assert exact ordering without knowing GitHub's algorithm + // but we can verify the parameter was processed (no errors) + } + } + }) + } + }) + + t.Run("Pagination Parameters", func(t *testing.T) { + // Test cases for pagination parameters + paginationTestCases := []struct { + name string + owner string + repo string + issueNumbers []int + first *int + last *int + after *string + before *string + expectError bool + description string + }{ + { + name: "Forward pagination with first parameter", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1, 2}, + first: intPtr(1), + description: "Test forward pagination using first parameter", + }, + { + name: "Backward pagination with last parameter", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1, 2}, + last: intPtr(1), + description: "Test backward pagination using last parameter", + }, + { + name: "Large repository pagination test", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1, 2, 3}, + first: intPtr(1), // Small page size + description: "Test pagination with larger repository", + }, + } + + for _, tc := range paginationTestCases { + t.Run(tc.name, func(t *testing.T) { + // Build arguments map + args := map[string]any{ + "owner": tc.owner, + "repo": tc.repo, + "issue_numbers": tc.issueNumbers, + } + + // Add pagination parameters if specified + if tc.first != nil { + args["limit"] = *tc.first // first maps to limit in our tool + } + if tc.last != nil { + args["last"] = *tc.last + } + if tc.after != nil { + args["after"] = *tc.after + } + if tc.before != nil { + args["before"] = *tc.before + } + + // Create request + request := mcp.CallToolRequest{} + request.Params.Name = "find_closing_pull_requests" + request.Params.Arguments = args + + t.Logf("Testing %s: %s", tc.name, tc.description) + t.Logf("Arguments: %+v", args) + + // Call the tool + resp, err := mcpClient.CallTool(ctx, request) + + if tc.expectError { + if err != nil { + t.Logf("Expected error occurred: %v", err) + return + } + require.True(t, resp.IsError, "Expected error response") + return + } + + require.NoError(t, err, "expected successful tool call") + require.False(t, resp.IsError, fmt.Sprintf("expected non-error response: %+v", resp)) + require.NotEmpty(t, resp.Content, "Expected response content") + + // Parse response + textContent, ok := resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected TextContent") + + var response struct { + Results []struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + IssueNumber int `json:"issue_number"` + TotalCount int `json:"total_count"` + ClosingPullRequests []struct { + Number int `json:"number"` + Title string `json:"title"` + State string `json:"state"` + } `json:"closing_pull_requests"` + PageInfo *struct { + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + StartCursor string `json:"startCursor"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo,omitempty"` + } `json:"results"` + } + + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err, "expected successful JSON parsing") + + // Verify response structure + require.NotEmpty(t, response.Results, "Expected at least one result") + + for i, result := range response.Results { + t.Logf("Result %d: Owner=%s, Repo=%s, Issue=%d, TotalCount=%d", + i+1, result.Owner, result.Repo, result.IssueNumber, result.TotalCount) + + // Verify pagination parameter effects + if tc.first != nil { + assert.LessOrEqual(t, len(result.ClosingPullRequests), *tc.first, + "Result count should not exceed 'first' parameter") + } + if tc.last != nil { + assert.LessOrEqual(t, len(result.ClosingPullRequests), *tc.last, + "Result count should not exceed 'last' parameter") + } + + // Log pagination info if present + if result.PageInfo != nil { + t.Logf(" PageInfo: HasNext=%t, HasPrev=%t", + result.PageInfo.HasNextPage, result.PageInfo.HasPreviousPage) + if result.PageInfo.StartCursor != "" { + t.Logf(" StartCursor: %s", result.PageInfo.StartCursor) + } + if result.PageInfo.EndCursor != "" { + t.Logf(" EndCursor: %s", result.PageInfo.EndCursor) + } + } + } + }) + } + }) + + t.Run("Error Validation", func(t *testing.T) { + // Test cases for parameter validation and error handling + errorTestCases := []struct { + name string + owner string + repo string + issueNumbers []int + args map[string]any + expectError bool + description string + }{ + { + name: "Conflicting limit and last parameters", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "limit": 5, + "last": 3, + }, + expectError: true, + description: "Should reject conflicting limit and last parameters", + }, + { + name: "Conflicting after and before cursors", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "after": "cursor1", + "before": "cursor2", + }, + expectError: true, + description: "Should reject conflicting after and before cursors", + }, + { + name: "Before cursor without last parameter", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "before": "cursor1", + }, + expectError: true, + description: "Should reject before cursor without last parameter", + }, + { + name: "After cursor with last parameter", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "after": "cursor1", + "last": 3, + }, + expectError: true, + description: "Should reject after cursor with last parameter", + }, + { + name: "Invalid limit range - too high", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "limit": 150, + }, + expectError: true, + description: "Should reject limit greater than 100", + }, + { + name: "Invalid last range - too high", + owner: "microsoft", + repo: "vscode", + issueNumbers: []int{1}, + args: map[string]any{ + "last": 150, + }, + expectError: true, + description: "Should reject last greater than 100", + }, + } + + for _, tc := range errorTestCases { + t.Run(tc.name, func(t *testing.T) { + // Build base arguments + args := map[string]any{ + "owner": tc.owner, + "repo": tc.repo, + "issue_numbers": tc.issueNumbers, + } + + // Add test-specific arguments + for key, value := range tc.args { + args[key] = value + } + + // Create request + request := mcp.CallToolRequest{} + request.Params.Name = "find_closing_pull_requests" + request.Params.Arguments = args + + t.Logf("Testing %s: %s", tc.name, tc.description) + t.Logf("Arguments: %+v", args) + + // Call the tool + resp, err := mcpClient.CallTool(ctx, request) + + if tc.expectError { + // We expect either an error or an error response + if err != nil { + t.Logf("Expected error occurred: %v", err) + return + } + require.True(t, resp.IsError, "Expected error response") + t.Logf("Expected error in response: %+v", resp) + + // Verify error content contains helpful information + if len(resp.Content) > 0 { + if textContent, ok := resp.Content[0].(mcp.TextContent); ok { + assert.NotEmpty(t, textContent.Text, "Error message should not be empty") + t.Logf("Error message: %s", textContent.Text) + } + } + return + } + + require.NoError(t, err, "expected successful tool call") + require.False(t, resp.IsError, "expected non-error response") + }) + } + }) +} + +// Helper functions for pointer creation +func boolPtr(b bool) *bool { + return &b +} + +func intPtr(i int) *int { + return &i +} + +func stringPtr(s string) *string { + return &s +} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 3cc7862b9..e99c898cc 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1332,10 +1332,16 @@ type ClosingPRNode struct { Merged githubv4.Boolean } -// ClosingPRsFragment represents the closedByPullRequestsReferences field +// ClosingPRsFragment represents the closedByPullRequestsReferences field with pagination info type ClosingPRsFragment struct { TotalCount githubv4.Int Nodes []ClosingPRNode + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } } // ClosingPullRequest represents a pull request in the response format @@ -1376,9 +1382,27 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla mcp.WithNumber("limit", mcp.Description("Maximum number of closing PRs to return per issue (default: 10, max: 100)"), ), + mcp.WithBoolean("includeClosedPrs", + mcp.Description("Include closed/merged pull requests in results (default: false)"), + ), + mcp.WithBoolean("orderByState", + mcp.Description("Order results by pull request state (default: false)"), + ), + mcp.WithBoolean("userLinkedOnly", + mcp.Description("Return only manually linked pull requests (default: false)"), + ), + mcp.WithString("after", + mcp.Description("Cursor for forward pagination (use with first/limit)"), + ), + mcp.WithString("before", + mcp.Description("Cursor for backward pagination (use with last)"), + ), + mcp.WithNumber("last", + mcp.Description("Number of results from end for backward pagination (max: 100)"), + ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // Parse limit parameter + // Parse pagination parameters limit := 10 // default if limitParam, exists := request.GetArguments()["limit"]; exists { if limitFloat, ok := limitParam.(float64); ok { @@ -1391,6 +1415,53 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } } + // Parse last parameter for backward pagination + last, err := OptionalIntParam(request, "last") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("last parameter error: %s", err.Error())), nil + } + if last != 0 && (last <= 0 || last > 100) { + return mcp.NewToolResultError("last must be between 1 and 100"), nil + } + + // Parse cursor parameters + after, err := OptionalParam[string](request, "after") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("after parameter error: %s", err.Error())), nil + } + before, err := OptionalParam[string](request, "before") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("before parameter error: %s", err.Error())), nil + } + + // Validate pagination parameter combinations + if last != 0 && limit != 10 { + return mcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together"), nil + } + if after != "" && before != "" { + return mcp.NewToolResultError("cannot use both 'after' and 'before' cursors together"), nil + } + if before != "" && last == 0 { + return mcp.NewToolResultError("'before' cursor requires 'last' parameter"), nil + } + if after != "" && last != 0 { + return mcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter"), nil + } + + // Parse filtering parameters + includeClosedPrs, err := OptionalParam[bool](request, "includeClosedPrs") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("includeClosedPrs parameter error: %s", err.Error())), nil + } + orderByState, err := OptionalParam[bool](request, "orderByState") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("orderByState parameter error: %s", err.Error())), nil + } + userLinkedOnly, err := OptionalParam[bool](request, "userLinkedOnly") + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("userLinkedOnly parameter error: %s", err.Error())), nil + } + // Get required parameters owner, err := RequiredParam[string](request, "owner") if err != nil { @@ -1416,10 +1487,29 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla return nil, fmt.Errorf("failed to get GraphQL client: %w", err) } + // Create pagination parameters struct + paginationParams := struct { + First int + Last int + After string + Before string + IncludeClosedPrs bool + OrderByState bool + UserLinkedOnly bool + }{ + First: limit, + Last: last, + After: after, + Before: before, + IncludeClosedPrs: includeClosedPrs, + OrderByState: orderByState, + UserLinkedOnly: userLinkedOnly, + } + // Process each issue number var results []map[string]interface{} for _, issueNum := range issueNumbers { - result, err := queryClosingPRsForIssue(ctx, client, owner, repo, issueNum, limit) + result, err := queryClosingPRsForIssueEnhanced(ctx, client, owner, repo, issueNum, paginationParams) if err != nil { // Add error result for this issue results = append(results, map[string]interface{}{ @@ -1449,24 +1539,58 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } } -// queryClosingPRsForIssue queries closing PRs for a single issue -func queryClosingPRsForIssue(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber, limit int) (map[string]interface{}, error) { +// queryClosingPRsForIssueEnhanced queries closing PRs for a single issue with enhanced parameters +func queryClosingPRsForIssueEnhanced(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber int, params struct { + First int + Last int + After string + Before string + IncludeClosedPrs bool + OrderByState bool + UserLinkedOnly bool +}) (map[string]interface{}, error) { // Define the GraphQL query for this specific issue var query struct { Repository struct { Issue struct { - ClosedByPullRequestsReferences ClosingPRsFragment `graphql:"closedByPullRequestsReferences(first: $limit)"` + ClosedByPullRequestsReferences ClosingPRsFragment `graphql:"closedByPullRequestsReferences(first: $first, last: $last, after: $after, before: $before, includeClosedPrs: $includeClosedPrs, orderByState: $orderByState, userLinkedOnly: $userLinkedOnly)"` } `graphql:"issue(number: $number)"` } `graphql:"repository(owner: $owner, name: $repo)"` } + // Build variables map with conditional inclusion variables := map[string]any{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), "number": githubv4.Int(issueNumber), - "limit": githubv4.Int(limit), } + // Add pagination parameters conditionally + if params.Last != 0 { + variables["last"] = githubv4.Int(params.Last) + variables["first"] = (*githubv4.Int)(nil) + } else { + variables["first"] = githubv4.Int(params.First) + variables["last"] = (*githubv4.Int)(nil) + } + + if params.After != "" { + variables["after"] = githubv4.String(params.After) + } else { + variables["after"] = (*githubv4.String)(nil) + } + + if params.Before != "" { + variables["before"] = githubv4.String(params.Before) + } else { + variables["before"] = (*githubv4.String)(nil) + } + + // Add filtering parameters + variables["includeClosedPrs"] = githubv4.Boolean(params.IncludeClosedPrs) + variables["orderByState"] = githubv4.Boolean(params.OrderByState) + variables["userLinkedOnly"] = githubv4.Boolean(params.UserLinkedOnly) + err := client.Query(ctx, &query, variables) if err != nil { return nil, fmt.Errorf("failed to query issue: %w", err) @@ -1485,12 +1609,25 @@ func queryClosingPRsForIssue(ctx context.Context, client *githubv4.Client, owner }) } - // Return results in format consistent with other tools - return map[string]interface{}{ + // Build response with pagination info + result := map[string]interface{}{ "owner": owner, "repo": repo, "issue_number": issueNumber, "total_count": int(query.Repository.Issue.ClosedByPullRequestsReferences.TotalCount), "closing_pull_requests": closingPullRequests, - }, nil + } + + // Add pagination information if cursors are being used + if params.After != "" || params.Before != "" || params.Last != 0 { + pageInfo := query.Repository.Issue.ClosedByPullRequestsReferences.PageInfo + result["page_info"] = map[string]interface{}{ + "has_next_page": bool(pageInfo.HasNextPage), + "has_previous_page": bool(pageInfo.HasPreviousPage), + "start_cursor": string(pageInfo.StartCursor), + "end_cursor": string(pageInfo.EndCursor), + } + } + + return result, nil } From 13de1b91122fb546f7e86301fa102367c0fa16c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 14:37:02 +0000 Subject: [PATCH 06/13] Update docs --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index b40974e20..dbc1d0645 100644 --- a/README.md +++ b/README.md @@ -526,6 +526,18 @@ The following sets of tools are available (all are on by default): - `repo`: Repository name (string, required) - `title`: Issue title (string, required) +- **find_closing_pull_requests** - Find closing pull requests + - `after`: Cursor for forward pagination (use with first/limit) (string, optional) + - `before`: Cursor for backward pagination (use with last) (string, optional) + - `includeClosedPrs`: Include closed/merged pull requests in results (default: false) (boolean, optional) + - `issue_numbers`: Array of issue numbers within the specified repository (number[], required) + - `last`: Number of results from end for backward pagination (max: 100) (number, optional) + - `limit`: Maximum number of closing PRs to return per issue (default: 10, max: 100) (number, optional) + - `orderByState`: Order results by pull request state (default: false) (boolean, optional) + - `owner`: The owner of the repository (string, required) + - `repo`: The name of the repository (string, required) + - `userLinkedOnly`: Return only manually linked pull requests (default: false) (boolean, optional) + - **get_issue** - Get issue details - `issue_number`: The number of the issue (number, required) - `owner`: The owner of the repository (string, required) From 89778990c10c0ced597f1da93e26d21446cfa315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 14:51:33 +0000 Subject: [PATCH 07/13] Fix lint issue --- pkg/github/issues.go | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index e99c898cc..322e78d9d 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/http" "strings" "time" @@ -1558,19 +1559,35 @@ func queryClosingPRsForIssueEnhanced(ctx context.Context, client *githubv4.Clien } `graphql:"repository(owner: $owner, name: $repo)"` } - // Build variables map with conditional inclusion + // Validate issue number + if issueNumber < 0 || issueNumber > math.MaxInt32 { + return nil, fmt.Errorf("issue number %d is out of valid range", issueNumber) + } + issueNumber32 := int32(issueNumber) // safe: range-checked above + + // Validate pagination + if params.Last != 0 && (params.Last < 0 || params.Last > math.MaxInt32) { + return nil, fmt.Errorf("last parameter %d is out of valid range", params.Last) + } + if params.First < 0 || params.First > math.MaxInt32 { + return nil, fmt.Errorf("first parameter %d is out of valid range", params.First) + } + + first32 := int32(params.First) + last32 := int32(params.Last) + + // Build variables map variables := map[string]any{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), - "number": githubv4.Int(issueNumber), + "number": githubv4.Int(issueNumber32), } - // Add pagination parameters conditionally - if params.Last != 0 { - variables["last"] = githubv4.Int(params.Last) + if last32 != 0 { + variables["last"] = githubv4.Int(last32) variables["first"] = (*githubv4.Int)(nil) } else { - variables["first"] = githubv4.Int(params.First) + variables["first"] = githubv4.Int(first32) variables["last"] = (*githubv4.Int)(nil) } From 02f356d79e827d6140ad6cdf01345392aee5f15b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 16:11:35 +0000 Subject: [PATCH 08/13] Address review comments --- e2e/e2e_test.go | 11 ++++------ pkg/github/issues.go | 50 +++++++++++++++++++++++--------------------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 478d4678f..61644c88a 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1724,10 +1724,9 @@ func TestFindClosingPullRequests(t *testing.T) { // Parse the JSON response var response struct { Results []struct { - Issue string `json:"issue"` Owner string `json:"owner"` Repo string `json:"repo"` - IssueNumber int `json:"issueNumber"` + IssueNumber int `json:"issue_number"` ClosingPullRequests []struct { Number int `json:"number"` Title string `json:"title"` @@ -1750,7 +1749,6 @@ func TestFindClosingPullRequests(t *testing.T) { // Log and verify each result for i, result := range response.Results { t.Logf("Result %d:", i+1) - t.Logf(" Issue: %s", result.Issue) t.Logf(" Owner: %s, Repo: %s, Number: %d", result.Owner, result.Repo, result.IssueNumber) t.Logf(" Total closing PRs: %d", result.TotalCount) if result.Error != "" { @@ -1758,7 +1756,6 @@ func TestFindClosingPullRequests(t *testing.T) { } // Verify basic structure - assert.NotEmpty(t, result.Issue, "Issue reference should not be empty") assert.NotEmpty(t, result.Owner, "Owner should not be empty") assert.NotEmpty(t, result.Repo, "Repo should not be empty") assert.Greater(t, result.IssueNumber, 0, "Issue number should be positive") @@ -1778,8 +1775,8 @@ func TestFindClosingPullRequests(t *testing.T) { assert.NotEmpty(t, pr.URL, "PR URL should not be empty") } - // The actual count of closing PRs should match the returned array length - assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should match TotalCount") + // The number of closing PRs in this page should be less than or equal to the total count + assert.LessOrEqual(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should not exceed TotalCount") } }) } @@ -1935,7 +1932,7 @@ func TestFindClosingPullRequestsGraphQLParameters(t *testing.T) { assert.Equal(t, tc.owner, result.Owner, "Owner should match request") assert.Equal(t, tc.repo, result.Repo, "Repo should match request") assert.Contains(t, tc.issueNumbers, result.IssueNumber, "Issue number should be in request") - assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "TotalCount should match array length") + assert.LessOrEqual(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should not exceed TotalCount") // Parameter-specific validations if tc.includeClosedPrs != nil && *tc.includeClosedPrs == false { diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 322e78d9d..1705cccf8 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "math" "net/http" "strings" "time" @@ -1355,6 +1354,11 @@ type ClosingPullRequest struct { Merged bool `json:"merged"` } +const ( + // DefaultClosingPRsLimit is the default number of closing PRs to return per issue + DefaultClosingPRsLimit = 10 +) + // FindClosingPullRequests creates a tool to find pull requests that closed specific issues func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("find_closing_pull_requests", @@ -1404,15 +1408,17 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Parse pagination parameters - limit := 10 // default + limit := DefaultClosingPRsLimit // default + limitExplicitlySet := false if limitParam, exists := request.GetArguments()["limit"]; exists { + limitExplicitlySet = true if limitFloat, ok := limitParam.(float64); ok { limit = int(limitFloat) if limit <= 0 || limit > 100 { - return mcp.NewToolResultError("limit must be between 1 and 100"), nil + return mcp.NewToolResultError("limit must be between 1 and 100 inclusive"), nil } } else { - return mcp.NewToolResultError("limit must be a number"), nil + return mcp.NewToolResultError("limit must be a number between 1 and 100"), nil } } @@ -1422,7 +1428,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla return mcp.NewToolResultError(fmt.Sprintf("last parameter error: %s", err.Error())), nil } if last != 0 && (last <= 0 || last > 100) { - return mcp.NewToolResultError("last must be between 1 and 100"), nil + return mcp.NewToolResultError("last must be between 1 and 100 inclusive for backward pagination"), nil } // Parse cursor parameters @@ -1436,17 +1442,17 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla } // Validate pagination parameter combinations - if last != 0 && limit != 10 { - return mcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together"), nil + if last != 0 && limitExplicitlySet { + return mcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together - use 'limit' for forward pagination or 'last' for backward pagination"), nil } if after != "" && before != "" { - return mcp.NewToolResultError("cannot use both 'after' and 'before' cursors together"), nil + return mcp.NewToolResultError("cannot use both 'after' and 'before' cursors together - use 'after' for forward pagination or 'before' for backward pagination"), nil } if before != "" && last == 0 { - return mcp.NewToolResultError("'before' cursor requires 'last' parameter"), nil + return mcp.NewToolResultError("'before' cursor requires 'last' parameter for backward pagination"), nil } if after != "" && last != 0 { - return mcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter"), nil + return mcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter - use 'after' with 'limit' for forward pagination"), nil } // Parse filtering parameters @@ -1479,7 +1485,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla // Validate that issue_numbers is not empty if len(issueNumbers) == 0 { - return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty"), nil + return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty - provide at least one issue number"), nil } // Get GraphQL client @@ -1559,35 +1565,31 @@ func queryClosingPRsForIssueEnhanced(ctx context.Context, client *githubv4.Clien } `graphql:"repository(owner: $owner, name: $repo)"` } - // Validate issue number - if issueNumber < 0 || issueNumber > math.MaxInt32 { + // Validate issue number (basic bounds check) + if issueNumber < 0 { return nil, fmt.Errorf("issue number %d is out of valid range", issueNumber) } - issueNumber32 := int32(issueNumber) // safe: range-checked above - // Validate pagination - if params.Last != 0 && (params.Last < 0 || params.Last > math.MaxInt32) { + // Validate pagination parameters (basic bounds check) + if params.Last < 0 { return nil, fmt.Errorf("last parameter %d is out of valid range", params.Last) } - if params.First < 0 || params.First > math.MaxInt32 { + if params.First < 0 { return nil, fmt.Errorf("first parameter %d is out of valid range", params.First) } - first32 := int32(params.First) - last32 := int32(params.Last) - // Build variables map variables := map[string]any{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), - "number": githubv4.Int(issueNumber32), + "number": githubv4.Int(issueNumber), // #nosec G115 - issueNumber validated to be positive } - if last32 != 0 { - variables["last"] = githubv4.Int(last32) + if params.Last != 0 { + variables["last"] = githubv4.Int(params.Last) // #nosec G115 - params.Last validated to be positive variables["first"] = (*githubv4.Int)(nil) } else { - variables["first"] = githubv4.Int(first32) + variables["first"] = githubv4.Int(params.First) // #nosec G115 - params.First validated to be positive variables["last"] = (*githubv4.Int)(nil) } From 34b53dba96dc302bf321d4207e79d73ff4180436 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Mon, 4 Aug 2025 16:16:59 +0000 Subject: [PATCH 09/13] Adjust for actual GitHub API limits --- pkg/github/issues.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 1705cccf8..d58df41af 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1356,7 +1356,8 @@ type ClosingPullRequest struct { const ( // DefaultClosingPRsLimit is the default number of closing PRs to return per issue - DefaultClosingPRsLimit = 10 + // Aligned with GitHub GraphQL API default of 100 items per page + DefaultClosingPRsLimit = 100 ) // FindClosingPullRequests creates a tool to find pull requests that closed specific issues @@ -1414,11 +1415,11 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla limitExplicitlySet = true if limitFloat, ok := limitParam.(float64); ok { limit = int(limitFloat) - if limit <= 0 || limit > 100 { - return mcp.NewToolResultError("limit must be between 1 and 100 inclusive"), nil + if limit <= 0 || limit > 250 { + return mcp.NewToolResultError("limit must be between 1 and 250 inclusive (GitHub GraphQL API maximum)"), nil } } else { - return mcp.NewToolResultError("limit must be a number between 1 and 100"), nil + return mcp.NewToolResultError("limit must be a number between 1 and 250 (GitHub GraphQL API maximum)"), nil } } @@ -1427,8 +1428,8 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla if err != nil { return mcp.NewToolResultError(fmt.Sprintf("last parameter error: %s", err.Error())), nil } - if last != 0 && (last <= 0 || last > 100) { - return mcp.NewToolResultError("last must be between 1 and 100 inclusive for backward pagination"), nil + if last != 0 && (last <= 0 || last > 250) { + return mcp.NewToolResultError("last must be between 1 and 250 inclusive for backward pagination (GitHub GraphQL API maximum)"), nil } // Parse cursor parameters From 2aa02b25f91ab97f4bc0cd8ff9e864663a39e261 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Tue, 5 Aug 2025 08:50:35 +0000 Subject: [PATCH 10/13] Update limit assertions and comments --- README.md | 4 ++-- e2e/e2e_test.go | 10 +++++----- pkg/github/issues.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index dbc1d0645..f4d38f3fe 100644 --- a/README.md +++ b/README.md @@ -531,8 +531,8 @@ The following sets of tools are available (all are on by default): - `before`: Cursor for backward pagination (use with last) (string, optional) - `includeClosedPrs`: Include closed/merged pull requests in results (default: false) (boolean, optional) - `issue_numbers`: Array of issue numbers within the specified repository (number[], required) - - `last`: Number of results from end for backward pagination (max: 100) (number, optional) - - `limit`: Maximum number of closing PRs to return per issue (default: 10, max: 100) (number, optional) + - `last`: Number of results from end for backward pagination (max: 250) (number, optional) + - `limit`: Maximum number of closing PRs to return per issue (default: 100, max: 250) (number, optional) - `orderByState`: Order results by pull request state (default: false) (boolean, optional) - `owner`: The owner of the repository (string, required) - `repo`: The name of the repository (string, required) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 61644c88a..de6ee5fa9 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1674,7 +1674,7 @@ func TestFindClosingPullRequests(t *testing.T) { owner: "octocat", repo: "Hello-World", issueNumbers: []int{1}, - limit: 150, + limit: 251, expectError: true, }, } @@ -2167,10 +2167,10 @@ func TestFindClosingPullRequestsGraphQLParameters(t *testing.T) { repo: "vscode", issueNumbers: []int{1}, args: map[string]any{ - "limit": 150, + "limit": 251, }, expectError: true, - description: "Should reject limit greater than 100", + description: "Should reject limit greater than 250", }, { name: "Invalid last range - too high", @@ -2178,10 +2178,10 @@ func TestFindClosingPullRequestsGraphQLParameters(t *testing.T) { repo: "vscode", issueNumbers: []int{1}, args: map[string]any{ - "last": 150, + "last": 251, }, expectError: true, - description: "Should reject last greater than 100", + description: "Should reject last greater than 250", }, } diff --git a/pkg/github/issues.go b/pkg/github/issues.go index d58df41af..2ab56ab88 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1386,7 +1386,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla ), ), mcp.WithNumber("limit", - mcp.Description("Maximum number of closing PRs to return per issue (default: 10, max: 100)"), + mcp.Description("Maximum number of closing PRs to return per issue (default: 100, max: 250)"), ), mcp.WithBoolean("includeClosedPrs", mcp.Description("Include closed/merged pull requests in results (default: false)"), @@ -1404,7 +1404,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla mcp.Description("Cursor for backward pagination (use with last)"), ), mcp.WithNumber("last", - mcp.Description("Number of results from end for backward pagination (max: 100)"), + mcp.Description("Number of results from end for backward pagination (max: 250)"), ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { From 230ca60c2aa27744e0f5b6db4b7406b04bf95ed9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Tue, 5 Aug 2025 11:27:17 +0200 Subject: [PATCH 11/13] Update pkg/github/server.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index 933bdb914..889c8ef43 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -202,7 +202,7 @@ func OptionalIntArrayParam(r mcp.CallToolRequest, p string) ([]int, error) { case int64: intSlice[i] = int(num) default: - return []int{}, fmt.Errorf("parameter %s is not of type number, is %T", p, v) + return []int{}, fmt.Errorf("parameter %s array element at index %d is not of type number, is %T", p, i, v) } } return intSlice, nil From 596ce58b86ce8847a103656e9b03820e264a328c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Tue, 5 Aug 2025 10:10:56 +0000 Subject: [PATCH 12/13] Use constant for limit --- pkg/github/issues.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 2ab56ab88..5ff0ad1d8 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1358,6 +1358,7 @@ const ( // DefaultClosingPRsLimit is the default number of closing PRs to return per issue // Aligned with GitHub GraphQL API default of 100 items per page DefaultClosingPRsLimit = 100 + MaxGraphQLPageSize = 250 // Maximum page size for GitHub GraphQL API ) // FindClosingPullRequests creates a tool to find pull requests that closed specific issues @@ -1386,7 +1387,11 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla ), ), mcp.WithNumber("limit", - mcp.Description("Maximum number of closing PRs to return per issue (default: 100, max: 250)"), + mcp.Description(fmt.Sprintf( + "Maximum number of closing PRs to return per issue (default: %d, max: %d)", + DefaultClosingPRsLimit, + MaxGraphQLPageSize, + )), ), mcp.WithBoolean("includeClosedPrs", mcp.Description("Include closed/merged pull requests in results (default: false)"), @@ -1404,7 +1409,10 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla mcp.Description("Cursor for backward pagination (use with last)"), ), mcp.WithNumber("last", - mcp.Description("Number of results from end for backward pagination (max: 250)"), + mcp.Description(fmt.Sprintf( + "Number of results from end for backward pagination (max: %d)", + MaxGraphQLPageSize, + )), ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ -1415,11 +1423,11 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla limitExplicitlySet = true if limitFloat, ok := limitParam.(float64); ok { limit = int(limitFloat) - if limit <= 0 || limit > 250 { - return mcp.NewToolResultError("limit must be between 1 and 250 inclusive (GitHub GraphQL API maximum)"), nil + if limit <= 0 || limit > MaxGraphQLPageSize { + return mcp.NewToolResultError(fmt.Sprintf("limit must be between 1 and %d inclusive (GitHub GraphQL API maximum)", MaxGraphQLPageSize)), nil } } else { - return mcp.NewToolResultError("limit must be a number between 1 and 250 (GitHub GraphQL API maximum)"), nil + return mcp.NewToolResultError(fmt.Sprintf("limit must be a number between 1 and %d (GitHub GraphQL API maximum)", MaxGraphQLPageSize)), nil } } @@ -1428,8 +1436,8 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla if err != nil { return mcp.NewToolResultError(fmt.Sprintf("last parameter error: %s", err.Error())), nil } - if last != 0 && (last <= 0 || last > 250) { - return mcp.NewToolResultError("last must be between 1 and 250 inclusive for backward pagination (GitHub GraphQL API maximum)"), nil + if last != 0 && (last <= 0 || last > MaxGraphQLPageSize) { + return mcp.NewToolResultError(fmt.Sprintf("last must be between 1 and %d inclusive for backward pagination (GitHub GraphQL API maximum)", MaxGraphQLPageSize)), nil } // Parse cursor parameters From 94af699ad2ec5be7213d76e8bd7f8c731bf5a13e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Spie=C3=9F?= Date: Tue, 5 Aug 2025 10:11:08 +0000 Subject: [PATCH 13/13] Add new RequiredIntArrayParam helper --- pkg/github/issues.go | 7 +-- pkg/github/server.go | 44 ++++++++++++++++ pkg/github/server_test.go | 104 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 6 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 5ff0ad1d8..69e4dea27 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1487,16 +1487,11 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla if err != nil { return mcp.NewToolResultError(fmt.Sprintf("repo parameter error: %s", err.Error())), nil } - issueNumbers, err := OptionalIntArrayParam(request, "issue_numbers") + issueNumbers, err := RequiredIntArrayParam(request, "issue_numbers") if err != nil { return mcp.NewToolResultError(fmt.Sprintf("issue_numbers parameter error: %s", err.Error())), nil } - // Validate that issue_numbers is not empty - if len(issueNumbers) == 0 { - return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty - provide at least one issue number"), nil - } - // Get GraphQL client client, err := getGQLClient(ctx) if err != nil { diff --git a/pkg/github/server.go b/pkg/github/server.go index 889c8ef43..a06083627 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -99,6 +99,50 @@ func RequiredInt(r mcp.CallToolRequest, p string) (int, error) { return int(v), nil } +// RequiredIntArrayParam is a helper function that can be used to fetch a required integer array parameter from the request. +// It does the following checks: +// 1. Checks if the parameter is present in the request +// 2. Checks if the parameter is an array and each element can be converted to int +// 3. Checks if the array is not empty +func RequiredIntArrayParam(r mcp.CallToolRequest, p string) ([]int, error) { + // Check if the parameter is present in the request + if _, ok := r.GetArguments()[p]; !ok { + return nil, fmt.Errorf("missing required parameter: %s", p) + } + + switch v := r.GetArguments()[p].(type) { + case nil: + return nil, fmt.Errorf("missing required parameter: %s", p) + case []int: + if len(v) == 0 { + return nil, fmt.Errorf("parameter %s cannot be empty", p) + } + return v, nil + case []any: + if len(v) == 0 { + return nil, fmt.Errorf("parameter %s cannot be empty", p) + } + intSlice := make([]int, len(v)) + for i, elem := range v { + switch num := elem.(type) { + case float64: + intSlice[i] = int(num) + case int: + intSlice[i] = num + case int32: + intSlice[i] = int(num) + case int64: + intSlice[i] = int(num) + default: + return nil, fmt.Errorf("parameter %s contains non-numeric value, element %d is %T", p, i, elem) + } + } + return intSlice, nil + default: + return nil, fmt.Errorf("parameter %s is not an array, is %T", p, r.GetArguments()[p]) + } +} + // OptionalParam is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request, if not, it returns its zero-value diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 047463650..f96f934bf 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -564,6 +564,110 @@ func TestOptionalIntArrayParam(t *testing.T) { } } +func TestRequiredIntArrayParam(t *testing.T) { + tests := []struct { + name string + params map[string]interface{} + paramName string + expected []int + expectError bool + }{ + { + name: "parameter not in request", + params: map[string]any{}, + paramName: "numbers", + expected: nil, + expectError: true, + }, + { + name: "valid any array parameter with float64", + params: map[string]any{ + "numbers": []any{float64(1), float64(2), float64(3)}, + }, + paramName: "numbers", + expected: []int{1, 2, 3}, + expectError: false, + }, + { + name: "valid int array parameter", + params: map[string]any{ + "numbers": []int{1, 2, 3}, + }, + paramName: "numbers", + expected: []int{1, 2, 3}, + expectError: false, + }, + { + name: "mixed numeric types", + params: map[string]any{ + "numbers": []any{float64(1), int(2), int32(3), int64(4)}, + }, + paramName: "numbers", + expected: []int{1, 2, 3, 4}, + expectError: false, + }, + { + name: "invalid type in array", + params: map[string]any{ + "numbers": []any{float64(1), "not a number"}, + }, + paramName: "numbers", + expected: nil, + expectError: true, + }, + { + name: "nil value", + params: map[string]any{ + "numbers": nil, + }, + paramName: "numbers", + expected: nil, + expectError: true, + }, + { + name: "empty array", + params: map[string]any{ + "numbers": []any{}, + }, + paramName: "numbers", + expected: nil, + expectError: true, + }, + { + name: "empty int array", + params: map[string]any{ + "numbers": []int{}, + }, + paramName: "numbers", + expected: nil, + expectError: true, + }, + { + name: "wrong parameter type", + params: map[string]any{ + "numbers": "not an array", + }, + paramName: "numbers", + expected: nil, + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + request := createMCPRequest(tc.params) + result, err := RequiredIntArrayParam(request, tc.paramName) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expected, result) + } + }) + } +} + func TestOptionalPaginationParams(t *testing.T) { tests := []struct { name string