Skip to content

Commit 24ce75e

Browse files
gokhanarkangokhanarkan
authored
Merge branch 'main' into gokhanarkan/fides-issue-read
2 parents 25f1537 + 9ad99c5 commit 24ce75e

7 files changed

Lines changed: 257 additions & 39 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1100,7 +1100,7 @@ The following sets of tools are available:
11001100
3. get_status - Get combined commit status of a head commit in a pull request.
11011101
4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.
11021102
5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.
1103-
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.
1103+
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.
11041104
7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.
11051105
8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.
11061106
(string, required)

‎pkg/github/__toolsnaps__/pull_request_read.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"inputSchema": {
88
"properties": {
99
"method": {
10-
"description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get combined commit status of a head commit in a pull request.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.\n 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.\n 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n 8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.\n",
10+
"description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get combined commit status of a head commit in a pull request.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.\n 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.\n 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n 8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.\n",
1111
"enum": [
1212
"get",
1313
"get_diff",

‎pkg/github/__toolsnaps__/update_issue_type.snap‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@
2020
"description": "Repository owner (username or organization)",
2121
"type": "string"
2222
},
23+
"rationale": {
24+
"description": "One concise sentence explaining what specifically about the issue led you to choose this type. State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature).",
25+
"maxLength": 280,
26+
"type": "string"
27+
},
2328
"repo": {
2429
"description": "Repository name",
2530
"type": "string"

‎pkg/github/granular_tools_test.go‎

Lines changed: 97 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package github
33
import (
44
"context"
55
"net/http"
6+
"strings"
67
"testing"
78

89
"github.com/github/github-mcp-server/internal/githubv4mock"
@@ -304,24 +305,103 @@ func TestGranularUpdateIssueMilestone(t *testing.T) {
304305
}
305306

306307
func TestGranularUpdateIssueType(t *testing.T) {
307-
client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
308-
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
309-
"type": "bug",
310-
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})),
311-
}))
312-
deps := BaseDeps{Client: client}
313-
serverTool := GranularUpdateIssueType(translations.NullTranslationHelper)
314-
handler := serverTool.Handler(deps)
308+
tests := []struct {
309+
name string
310+
requestArgs map[string]any
311+
expectedReq map[string]any
312+
}{
313+
{
314+
name: "type only",
315+
requestArgs: map[string]any{
316+
"owner": "owner",
317+
"repo": "repo",
318+
"issue_number": float64(1),
319+
"issue_type": "bug",
320+
},
321+
expectedReq: map[string]any{
322+
"type": "bug",
323+
},
324+
},
325+
{
326+
name: "type with rationale",
327+
requestArgs: map[string]any{
328+
"owner": "owner",
329+
"repo": "repo",
330+
"issue_number": float64(1),
331+
"issue_type": "feature",
332+
"rationale": " This issue requests a new capability ",
333+
},
334+
expectedReq: map[string]any{
335+
"type": map[string]any{
336+
"value": "feature",
337+
"rationale": "This issue requests a new capability",
338+
},
339+
},
340+
},
341+
}
315342

316-
request := createMCPRequest(map[string]any{
317-
"owner": "owner",
318-
"repo": "repo",
319-
"issue_number": float64(1),
320-
"issue_type": "bug",
321-
})
322-
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
323-
require.NoError(t, err)
324-
assert.False(t, result.IsError)
343+
for _, tc := range tests {
344+
t.Run(tc.name, func(t *testing.T) {
345+
client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
346+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq).
347+
andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})),
348+
}))
349+
deps := BaseDeps{Client: client}
350+
serverTool := GranularUpdateIssueType(translations.NullTranslationHelper)
351+
handler := serverTool.Handler(deps)
352+
353+
request := createMCPRequest(tc.requestArgs)
354+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
355+
require.NoError(t, err)
356+
assert.False(t, result.IsError)
357+
})
358+
}
359+
}
360+
361+
func TestGranularUpdateIssueTypeInvalidRationale(t *testing.T) {
362+
tests := []struct {
363+
name string
364+
requestArgs map[string]any
365+
expectedErrText string
366+
}{
367+
{
368+
name: "rationale wrong type",
369+
requestArgs: map[string]any{
370+
"owner": "owner",
371+
"repo": "repo",
372+
"issue_number": float64(1),
373+
"issue_type": "feature",
374+
"rationale": float64(123),
375+
},
376+
expectedErrText: "parameter rationale is not of type string, is float64",
377+
},
378+
{
379+
name: "rationale too long",
380+
requestArgs: map[string]any{
381+
"owner": "owner",
382+
"repo": "repo",
383+
"issue_number": float64(1),
384+
"issue_type": "feature",
385+
"rationale": strings.Repeat("a", 281),
386+
},
387+
expectedErrText: "parameter rationale must be 280 characters or less",
388+
},
389+
}
390+
391+
for _, tc := range tests {
392+
t.Run(tc.name, func(t *testing.T) {
393+
deps := BaseDeps{Client: gogithub.NewClient(MockHTTPClientWithHandlers(nil))}
394+
serverTool := GranularUpdateIssueType(translations.NullTranslationHelper)
395+
handler := serverTool.Handler(deps)
396+
397+
request := createMCPRequest(tc.requestArgs)
398+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
399+
require.NoError(t, err)
400+
401+
errorContent := getErrorResult(t, result)
402+
assert.Contains(t, errorContent.Text, tc.expectedErrText)
403+
})
404+
}
325405
}
326406

327407
func TestGranularUpdateIssueState(t *testing.T) {

‎pkg/github/issues_granular.go‎

Lines changed: 116 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -309,27 +309,131 @@ func GranularUpdateIssueMilestone(t translations.TranslationHelperFunc) inventor
309309
)
310310
}
311311

312+
// issueTypeWithRationale represents the object form of the issue type field,
313+
// allowing a rationale to be sent alongside the type name.
314+
type issueTypeWithRationale struct {
315+
Value string `json:"value"`
316+
Rationale string `json:"rationale"`
317+
}
318+
319+
// issueTypeUpdateRequest is a custom request body for updating an issue type
320+
// with an optional rationale, using the object form that the REST API accepts.
321+
type issueTypeUpdateRequest struct {
322+
Type issueTypeWithRationale `json:"type"`
323+
}
324+
312325
// GranularUpdateIssueType creates a tool to update an issue's type.
313326
func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.ServerTool {
314-
return issueUpdateTool(t,
315-
"update_issue_type",
316-
"Update the type of an existing issue (e.g. 'bug', 'feature').",
317-
"Update Issue Type",
318-
map[string]*jsonschema.Schema{
319-
"issue_type": {
320-
Type: "string",
321-
Description: "The issue type to set",
327+
st := NewTool(
328+
ToolsetMetadataIssues,
329+
mcp.Tool{
330+
Name: "update_issue_type",
331+
Description: t("TOOL_UPDATE_ISSUE_TYPE_DESCRIPTION", "Update the type of an existing issue (e.g. 'bug', 'feature')."),
332+
Annotations: &mcp.ToolAnnotations{
333+
Title: t("TOOL_UPDATE_ISSUE_TYPE_USER_TITLE", "Update Issue Type"),
334+
ReadOnlyHint: false,
335+
DestructiveHint: jsonschema.Ptr(false),
336+
OpenWorldHint: jsonschema.Ptr(true),
337+
},
338+
InputSchema: &jsonschema.Schema{
339+
Type: "object",
340+
Properties: map[string]*jsonschema.Schema{
341+
"owner": {
342+
Type: "string",
343+
Description: "Repository owner (username or organization)",
344+
},
345+
"repo": {
346+
Type: "string",
347+
Description: "Repository name",
348+
},
349+
"issue_number": {
350+
Type: "number",
351+
Description: "The issue number to update",
352+
Minimum: jsonschema.Ptr(1.0),
353+
},
354+
"issue_type": {
355+
Type: "string",
356+
Description: "The issue type to set",
357+
},
358+
"rationale": {
359+
Type: "string",
360+
Description: "One concise sentence explaining what specifically about the issue led you to choose this type. " +
361+
"State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature).",
362+
MaxLength: jsonschema.Ptr(280),
363+
},
364+
},
365+
Required: []string{"owner", "repo", "issue_number", "issue_type"},
322366
},
323367
},
324-
[]string{"issue_type"},
325-
func(args map[string]any) (*github.IssueRequest, error) {
368+
[]scopes.Scope{scopes.Repo},
369+
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
370+
owner, err := RequiredParam[string](args, "owner")
371+
if err != nil {
372+
return utils.NewToolResultError(err.Error()), nil, nil
373+
}
374+
repo, err := RequiredParam[string](args, "repo")
375+
if err != nil {
376+
return utils.NewToolResultError(err.Error()), nil, nil
377+
}
378+
issueNumber, err := RequiredInt(args, "issue_number")
379+
if err != nil {
380+
return utils.NewToolResultError(err.Error()), nil, nil
381+
}
326382
issueType, err := RequiredParam[string](args, "issue_type")
327383
if err != nil {
328-
return nil, err
384+
return utils.NewToolResultError(err.Error()), nil, nil
385+
}
386+
rationale, err := OptionalParam[string](args, "rationale")
387+
if err != nil {
388+
return utils.NewToolResultError(err.Error()), nil, nil
389+
}
390+
rationale = strings.TrimSpace(rationale)
391+
if len([]rune(rationale)) > 280 {
392+
return utils.NewToolResultError("parameter rationale must be 280 characters or less"), nil, nil
393+
}
394+
395+
client, err := deps.GetClient(ctx)
396+
if err != nil {
397+
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
398+
}
399+
400+
var body any
401+
if rationale != "" {
402+
body = &issueTypeUpdateRequest{
403+
Type: issueTypeWithRationale{
404+
Value: issueType,
405+
Rationale: rationale,
406+
},
407+
}
408+
} else {
409+
body = &github.IssueRequest{Type: &issueType}
410+
}
411+
412+
apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber)
413+
req, err := client.NewRequest("PATCH", apiURL, body)
414+
if err != nil {
415+
return utils.NewToolResultErrorFromErr("failed to create request", err), nil, nil
329416
}
330-
return &github.IssueRequest{Type: &issueType}, nil
417+
418+
issue := &github.Issue{}
419+
resp, err := client.Do(ctx, req, issue)
420+
if err != nil {
421+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", resp, err), nil, nil
422+
}
423+
defer func() { _ = resp.Body.Close() }()
424+
425+
r, err := json.Marshal(MinimalResponse{
426+
ID: fmt.Sprintf("%d", issue.GetID()),
427+
URL: issue.GetHTMLURL(),
428+
})
429+
if err != nil {
430+
return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil
431+
}
432+
return utils.NewToolResultText(string(r)), nil, nil
331433
},
332434
)
435+
st.FeatureFlagEnable = FeatureFlagIssuesGranular
436+
return st
333437
}
334438

335439
// GranularUpdateIssueState creates a tool to update an issue's state.

‎pkg/github/pullrequests.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ Possible options:
3636
3. get_status - Get combined commit status of a head commit in a pull request.
3737
4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.
3838
5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.
39-
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.
39+
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.
4040
7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.
4141
8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.
4242
`,
@@ -124,7 +124,7 @@ Possible options:
124124
result, err := GetPullRequestReviewComments(ctx, gqlClient, deps, owner, repo, pullNumber, cursorPagination)
125125
return result, nil, err
126126
case "get_reviews":
127-
result, err := GetPullRequestReviews(ctx, client, deps, owner, repo, pullNumber)
127+
result, err := GetPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination)
128128
return result, nil, err
129129
case "get_comments":
130130
result, err := GetIssueComments(ctx, client, deps, owner, repo, pullNumber, pagination)
@@ -478,14 +478,17 @@ func GetPullRequestReviewComments(ctx context.Context, gqlClient *githubv4.Clien
478478
return MarshalledTextResult(convertToMinimalReviewThreadsResponse(query)), nil
479479
}
480480

481-
func GetPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) {
481+
func GetPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) {
482482
cache, err := deps.GetRepoAccessCache(ctx)
483483
if err != nil {
484484
return nil, fmt.Errorf("failed to get repo access cache: %w", err)
485485
}
486486
ff := deps.GetFlags(ctx)
487487

488-
reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, pullNumber, nil)
488+
reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, pullNumber, &github.ListOptions{
489+
Page: pagination.Page,
490+
PerPage: pagination.PerPage,
491+
})
489492
if err != nil {
490493
return ghErrors.NewGitHubAPIErrorResponse(ctx,
491494
"failed to get pull request reviews",

‎pkg/github/pullrequests_test.go‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2050,13 +2050,39 @@ func Test_GetPullRequestReviews(t *testing.T) {
20502050
expectError: false,
20512051
expectedReviews: mockReviews,
20522052
},
2053+
{
2054+
name: "successful reviews fetch with pagination",
2055+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2056+
GetReposPullsReviewsByOwnerByRepoByPullNumber: expectQueryParams(t, map[string]string{
2057+
"page": "2",
2058+
"per_page": "10",
2059+
}).andThen(
2060+
mockResponse(t, http.StatusOK, mockReviews),
2061+
),
2062+
}),
2063+
requestArgs: map[string]any{
2064+
"method": "get_reviews",
2065+
"owner": "owner",
2066+
"repo": "repo",
2067+
"pullNumber": float64(42),
2068+
"page": float64(2),
2069+
"perPage": float64(10),
2070+
},
2071+
expectError: false,
2072+
expectedReviews: mockReviews,
2073+
},
20532074
{
20542075
name: "reviews fetch fails",
20552076
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2056-
GetReposPullsReviewsByOwnerByRepoByPullNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2057-
w.WriteHeader(http.StatusNotFound)
2058-
_, _ = w.Write([]byte(`{"message": "Not Found"}`))
2059-
}),
2077+
GetReposPullsReviewsByOwnerByRepoByPullNumber: expectQueryParams(t, map[string]string{
2078+
"page": "1",
2079+
"per_page": "30",
2080+
}).andThen(
2081+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2082+
w.WriteHeader(http.StatusNotFound)
2083+
_, _ = w.Write([]byte(`{"message": "Not Found"}`))
2084+
}),
2085+
),
20602086
}),
20612087
requestArgs: map[string]any{
20622088
"method": "get_reviews",

0 commit comments

Comments
 (0)