Skip to content

Commit 02f356d

Browse files
lumaxislumaxis
authored
Address review comments
1 parent 8977899 commit 02f356d

File tree

2 files changed

+30
-31
lines changed

2 files changed

+30
-31
lines changed

‎e2e/e2e_test.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1724,10 +1724,9 @@ func TestFindClosingPullRequests(t *testing.T) {
17241724
// Parse the JSON response
17251725
var response struct {
17261726
Results []struct {
1727-
Issue string `json:"issue"`
17281727
Owner string `json:"owner"`
17291728
Repo string `json:"repo"`
1730-
IssueNumber int `json:"issueNumber"`
1729+
IssueNumber int `json:"issue_number"`
17311730
ClosingPullRequests []struct {
17321731
Number int `json:"number"`
17331732
Title string `json:"title"`
@@ -1750,15 +1749,13 @@ func TestFindClosingPullRequests(t *testing.T) {
17501749
// Log and verify each result
17511750
for i, result := range response.Results {
17521751
t.Logf("Result %d:", i+1)
1753-
t.Logf(" Issue: %s", result.Issue)
17541752
t.Logf(" Owner: %s, Repo: %s, Number: %d", result.Owner, result.Repo, result.IssueNumber)
17551753
t.Logf(" Total closing PRs: %d", result.TotalCount)
17561754
if result.Error != "" {
17571755
t.Logf(" Error: %s", result.Error)
17581756
}
17591757

17601758
// Verify basic structure
1761-
assert.NotEmpty(t, result.Issue, "Issue reference should not be empty")
17621759
assert.NotEmpty(t, result.Owner, "Owner should not be empty")
17631760
assert.NotEmpty(t, result.Repo, "Repo should not be empty")
17641761
assert.Greater(t, result.IssueNumber, 0, "Issue number should be positive")
@@ -1778,8 +1775,8 @@ func TestFindClosingPullRequests(t *testing.T) {
17781775
assert.NotEmpty(t, pr.URL, "PR URL should not be empty")
17791776
}
17801777

1781-
// The actual count of closing PRs should match the returned array length
1782-
assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should match TotalCount")
1778+
// The number of closing PRs in this page should be less than or equal to the total count
1779+
assert.LessOrEqual(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should not exceed TotalCount")
17831780
}
17841781
})
17851782
}
@@ -1935,7 +1932,7 @@ func TestFindClosingPullRequestsGraphQLParameters(t *testing.T) {
19351932
assert.Equal(t, tc.owner, result.Owner, "Owner should match request")
19361933
assert.Equal(t, tc.repo, result.Repo, "Repo should match request")
19371934
assert.Contains(t, tc.issueNumbers, result.IssueNumber, "Issue number should be in request")
1938-
assert.Equal(t, len(result.ClosingPullRequests), result.TotalCount, "TotalCount should match array length")
1935+
assert.LessOrEqual(t, len(result.ClosingPullRequests), result.TotalCount, "ClosingPullRequests length should not exceed TotalCount")
19391936

19401937
// Parameter-specific validations
19411938
if tc.includeClosedPrs != nil && *tc.includeClosedPrs == false {

‎pkg/github/issues.go

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"encoding/json"
66
"fmt"
77
"io"
8-
"math"
98
"net/http"
109
"strings"
1110
"time"
@@ -1355,6 +1354,11 @@ type ClosingPullRequest struct {
13551354
Merged bool `json:"merged"`
13561355
}
13571356

1357+
const (
1358+
// DefaultClosingPRsLimit is the default number of closing PRs to return per issue
1359+
DefaultClosingPRsLimit = 10
1360+
)
1361+
13581362
// FindClosingPullRequests creates a tool to find pull requests that closed specific issues
13591363
func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) {
13601364
return mcp.NewTool("find_closing_pull_requests",
@@ -1404,15 +1408,17 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla
14041408
),
14051409
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
14061410
// Parse pagination parameters
1407-
limit := 10 // default
1411+
limit := DefaultClosingPRsLimit // default
1412+
limitExplicitlySet := false
14081413
if limitParam, exists := request.GetArguments()["limit"]; exists {
1414+
limitExplicitlySet = true
14091415
if limitFloat, ok := limitParam.(float64); ok {
14101416
limit = int(limitFloat)
14111417
if limit <= 0 || limit > 100 {
1412-
return mcp.NewToolResultError("limit must be between 1 and 100"), nil
1418+
return mcp.NewToolResultError("limit must be between 1 and 100 inclusive"), nil
14131419
}
14141420
} else {
1415-
return mcp.NewToolResultError("limit must be a number"), nil
1421+
return mcp.NewToolResultError("limit must be a number between 1 and 100"), nil
14161422
}
14171423
}
14181424

@@ -1422,7 +1428,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla
14221428
return mcp.NewToolResultError(fmt.Sprintf("last parameter error: %s", err.Error())), nil
14231429
}
14241430
if last != 0 && (last <= 0 || last > 100) {
1425-
return mcp.NewToolResultError("last must be between 1 and 100"), nil
1431+
return mcp.NewToolResultError("last must be between 1 and 100 inclusive for backward pagination"), nil
14261432
}
14271433

14281434
// Parse cursor parameters
@@ -1436,17 +1442,17 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla
14361442
}
14371443

14381444
// Validate pagination parameter combinations
1439-
if last != 0 && limit != 10 {
1440-
return mcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together"), nil
1445+
if last != 0 && limitExplicitlySet {
1446+
return mcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together - use 'limit' for forward pagination or 'last' for backward pagination"), nil
14411447
}
14421448
if after != "" && before != "" {
1443-
return mcp.NewToolResultError("cannot use both 'after' and 'before' cursors together"), nil
1449+
return mcp.NewToolResultError("cannot use both 'after' and 'before' cursors together - use 'after' for forward pagination or 'before' for backward pagination"), nil
14441450
}
14451451
if before != "" && last == 0 {
1446-
return mcp.NewToolResultError("'before' cursor requires 'last' parameter"), nil
1452+
return mcp.NewToolResultError("'before' cursor requires 'last' parameter for backward pagination"), nil
14471453
}
14481454
if after != "" && last != 0 {
1449-
return mcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter"), nil
1455+
return mcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter - use 'after' with 'limit' for forward pagination"), nil
14501456
}
14511457

14521458
// Parse filtering parameters
@@ -1479,7 +1485,7 @@ func FindClosingPullRequests(getGQLClient GetGQLClientFn, t translations.Transla
14791485

14801486
// Validate that issue_numbers is not empty
14811487
if len(issueNumbers) == 0 {
1482-
return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty"), nil
1488+
return mcp.NewToolResultError("issue_numbers parameter is required and cannot be empty - provide at least one issue number"), nil
14831489
}
14841490

14851491
// Get GraphQL client
@@ -1559,35 +1565,31 @@ func queryClosingPRsForIssueEnhanced(ctx context.Context, client *githubv4.Clien
15591565
} `graphql:"repository(owner: $owner, name: $repo)"`
15601566
}
15611567

1562-
// Validate issue number
1563-
if issueNumber < 0 || issueNumber > math.MaxInt32 {
1568+
// Validate issue number (basic bounds check)
1569+
if issueNumber < 0 {
15641570
return nil, fmt.Errorf("issue number %d is out of valid range", issueNumber)
15651571
}
1566-
issueNumber32 := int32(issueNumber) // safe: range-checked above
15671572

1568-
// Validate pagination
1569-
if params.Last != 0 && (params.Last < 0 || params.Last > math.MaxInt32) {
1573+
// Validate pagination parameters (basic bounds check)
1574+
if params.Last < 0 {
15701575
return nil, fmt.Errorf("last parameter %d is out of valid range", params.Last)
15711576
}
1572-
if params.First < 0 || params.First > math.MaxInt32 {
1577+
if params.First < 0 {
15731578
return nil, fmt.Errorf("first parameter %d is out of valid range", params.First)
15741579
}
15751580

1576-
first32 := int32(params.First)
1577-
last32 := int32(params.Last)
1578-
15791581
// Build variables map
15801582
variables := map[string]any{
15811583
"owner": githubv4.String(owner),
15821584
"repo": githubv4.String(repo),
1583-
"number": githubv4.Int(issueNumber32),
1585+
"number": githubv4.Int(issueNumber), // #nosec G115 - issueNumber validated to be positive
15841586
}
15851587

1586-
if last32 != 0 {
1587-
variables["last"] = githubv4.Int(last32)
1588+
if params.Last != 0 {
1589+
variables["last"] = githubv4.Int(params.Last) // #nosec G115 - params.Last validated to be positive
15881590
variables["first"] = (*githubv4.Int)(nil)
15891591
} else {
1590-
variables["first"] = githubv4.Int(first32)
1592+
variables["first"] = githubv4.Int(params.First) // #nosec G115 - params.First validated to be positive
15911593
variables["last"] = (*githubv4.Int)(nil)
15921594
}
15931595

0 commit comments

Comments
 (0)