Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit02f356d

Browse files
authored
Address review comments
1 parent8977899 commit02f356d

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
varresponsestruct {
17261726
Results []struct {
1727-
Issuestring`json:"issue"`
17281727
Ownerstring`json:"owner"`
17291728
Repostring`json:"repo"`
1730-
IssueNumberint`json:"issueNumber"`
1729+
IssueNumberint`json:"issue_number"`
17311730
ClosingPullRequests []struct {
17321731
Numberint`json:"number"`
17331732
Titlestring`json:"title"`
@@ -1750,15 +1749,13 @@ func TestFindClosingPullRequests(t *testing.T) {
17501749
// Log and verify each result
17511750
fori,result:=rangeresponse.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
ifresult.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-
// Theactual countof closing PRs shouldmatch the returned array length
1782-
assert.Equal(t,len(result.ClosingPullRequests),result.TotalCount,"ClosingPullRequests length shouldmatch TotalCount")
1778+
// Thenumberof closing PRsin this pageshouldbe less than or equal to the total count
1779+
assert.LessOrEqual(t,len(result.ClosingPullRequests),result.TotalCount,"ClosingPullRequests length shouldnot 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,"TotalCountshouldmatch array length")
1935+
assert.LessOrEqual(t,len(result.ClosingPullRequests),result.TotalCount,"ClosingPullRequests lengthshouldnot exceed TotalCount")
19391936

19401937
// Parameter-specific validations
19411938
iftc.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
Mergedbool`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
funcFindClosingPullRequests(getGQLClientGetGQLClientFn,t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) {
13601364
returnmcp.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
iflimitParam,exists:=request.GetArguments()["limit"];exists {
1414+
limitExplicitlySet=true
14091415
iflimitFloat,ok:=limitParam.(float64);ok {
14101416
limit=int(limitFloat)
14111417
iflimit<=0||limit>100 {
1412-
returnmcp.NewToolResultError("limit must be between 1 and 100"),nil
1418+
returnmcp.NewToolResultError("limit must be between 1 and 100 inclusive"),nil
14131419
}
14141420
}else {
1415-
returnmcp.NewToolResultError("limit must be a number"),nil
1421+
returnmcp.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
returnmcp.NewToolResultError(fmt.Sprintf("last parameter error: %s",err.Error())),nil
14231429
}
14241430
iflast!=0&& (last<=0||last>100) {
1425-
returnmcp.NewToolResultError("last must be between 1 and 100"),nil
1431+
returnmcp.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-
iflast!=0&&limit!=10 {
1440-
returnmcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together"),nil
1445+
iflast!=0&&limitExplicitlySet {
1446+
returnmcp.NewToolResultError("cannot use both 'limit' and 'last' parameters together - use 'limit' for forward pagination or 'last' for backward pagination"),nil
14411447
}
14421448
ifafter!=""&&before!="" {
1443-
returnmcp.NewToolResultError("cannot use both 'after' and 'before' cursors together"),nil
1449+
returnmcp.NewToolResultError("cannot use both 'after' and 'before' cursors together - use 'after' for forward pagination or 'before' for backward pagination"),nil
14441450
}
14451451
ifbefore!=""&&last==0 {
1446-
returnmcp.NewToolResultError("'before' cursor requires 'last' parameter"),nil
1452+
returnmcp.NewToolResultError("'before' cursor requires 'last' parameter for backward pagination"),nil
14471453
}
14481454
ifafter!=""&&last!=0 {
1449-
returnmcp.NewToolResultError("'after' cursor cannot be used with 'last' parameter"),nil
1455+
returnmcp.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
iflen(issueNumbers)==0 {
1482-
returnmcp.NewToolResultError("issue_numbers parameter is required and cannot be empty"),nil
1488+
returnmcp.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-
ifissueNumber<0||issueNumber>math.MaxInt32{
1568+
// Validate issue number (basic bounds check)
1569+
ifissueNumber<0 {
15641570
returnnil,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-
ifparams.Last!=0&& (params.Last<0||params.Last>math.MaxInt32) {
1573+
// Validate pagination parameters (basic bounds check)
1574+
ifparams.Last<0 {
15701575
returnnil,fmt.Errorf("last parameter %d is out of valid range",params.Last)
15711576
}
1572-
ifparams.First<0||params.First>math.MaxInt32{
1577+
ifparams.First<0 {
15731578
returnnil,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-
iflast32!=0 {
1587-
variables["last"]=githubv4.Int(last32)
1588+
ifparams.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)
[8]ページ先頭
©2009-2025 Movatter.jp