Skip to content

Commit ec35154

Browse files
authored
Merge branch 'main' into feat/git-blame-tool
2 parents d2ca606 + e2ff518 commit ec35154

7 files changed

Lines changed: 265 additions & 17 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/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",

pkg/github/repositories.go

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"strings"
1111

1212
ghErrors "github.com/github/github-mcp-server/pkg/errors"
13+
"github.com/github/github-mcp-server/pkg/ifc"
1314
"github.com/github/github-mcp-server/pkg/inventory"
1415
"github.com/github/github-mcp-server/pkg/octicons"
1516
"github.com/github/github-mcp-server/pkg/scopes"
@@ -682,6 +683,20 @@ func FetchRepoCollaborators(ctx context.Context, client *github.Client, owner, r
682683
return logins, nil
683684
}
684685

686+
// FetchRepoIsPrivate returns whether a repository is private. It is a thin
687+
// wrapper around the GitHub Repositories.Get endpoint provided as a shared
688+
// helper for IFC label computation across tools.
689+
func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo string) (bool, error) {
690+
r, resp, err := client.Repositories.Get(ctx, owner, repo)
691+
if resp != nil {
692+
defer func() { _ = resp.Body.Close() }()
693+
}
694+
if err != nil {
695+
return false, err
696+
}
697+
return r.GetPrivate(), nil
698+
}
699+
685700
// GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository.
686701
func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool {
687702
return NewTool(
@@ -754,6 +769,46 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
754769
return utils.NewToolResultError("failed to get GitHub client"), nil, nil
755770
}
756771

772+
// attachIFC adds the IFC label to a successful tool result when
773+
// InsidersMode is enabled. The visibility and (for private
774+
// repositories) collaborators lookups are performed lazily on
775+
// first use. If the visibility lookup fails we skip the label
776+
// rather than misclassify the result; the failure is not cached
777+
// so a later return path can retry. If only the collaborators
778+
// lookup fails for a private repo we fall back to the owner so
779+
// the reader set is never empty.
780+
var (
781+
ifcLabelKnown bool
782+
ifcIsPrivate bool
783+
ifcReaders []string
784+
)
785+
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
786+
if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode {
787+
return r
788+
}
789+
if !ifcLabelKnown {
790+
isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo)
791+
if err != nil {
792+
return r
793+
}
794+
ifcIsPrivate = isPrivate
795+
if ifcIsPrivate {
796+
if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil {
797+
ifcReaders = collaborators
798+
}
799+
if len(ifcReaders) == 0 {
800+
ifcReaders = []string{owner}
801+
}
802+
}
803+
ifcLabelKnown = true
804+
}
805+
if r.Meta == nil {
806+
r.Meta = mcp.Meta{}
807+
}
808+
r.Meta["ifc"] = ifc.LabelGetFileContents(ifcIsPrivate, ifcReaders)
809+
return r
810+
}
811+
757812
rawOpts, fallbackUsed, err := resolveGitReference(ctx, client, owner, repo, ref, sha)
758813
if err != nil {
759814
return utils.NewToolResultError(fmt.Sprintf("failed to resolve git reference: %s", err)), nil, nil
@@ -775,7 +830,8 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
775830
// The path does not point to a file or directory.
776831
// Instead let's try to find it in the Git Tree by matching the end of the path.
777832
if err != nil || (fileContent == nil && dirContent == nil) {
778-
return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
833+
res, data, err := matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
834+
return attachIFC(res), data, err
779835
}
780836

781837
if fileContent != nil && fileContent.SHA != nil {
@@ -805,7 +861,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
805861
Text: "",
806862
MIMEType: "text/plain",
807863
}
808-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
864+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
809865
}
810866

811867
// For files >= 1MB, return a ResourceLink instead of content
@@ -818,10 +874,10 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
818874
Title: fmt.Sprintf("File: %s", path),
819875
Size: &size,
820876
}
821-
return utils.NewToolResultResourceLink(
877+
return attachIFC(utils.NewToolResultResourceLink(
822878
fmt.Sprintf("File %s is too large to display (%d bytes). Use the download URL to fetch the content: %s (SHA: %s)%s",
823879
path, fileSize, fileContent.GetDownloadURL(), fileSHA, successNote),
824-
resourceLink), nil, nil
880+
resourceLink)), nil, nil
825881
}
826882

827883
// For files < 1MB, get content directly from Contents API
@@ -849,7 +905,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
849905
Text: content,
850906
MIMEType: contentType,
851907
}
852-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
908+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
853909
}
854910

855911
// Binary content - encode as base64 blob
@@ -859,14 +915,14 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
859915
Blob: []byte(blobContent),
860916
MIMEType: contentType,
861917
}
862-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result), nil, nil
918+
return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil
863919
} else if dirContent != nil {
864920
// file content or file SHA is nil which means it's a directory
865921
r, err := json.Marshal(dirContent)
866922
if err != nil {
867923
return utils.NewToolResultError("failed to marshal response"), nil, nil
868924
}
869-
return utils.NewToolResultText(string(r)), nil, nil
925+
return attachIFC(utils.NewToolResultText(string(r))), nil, nil
870926
}
871927

872928
return utils.NewToolResultError("failed to get file contents"), nil, nil

pkg/github/repositories_test.go

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,158 @@ func Test_GetFileContents(t *testing.T) {
479479
}
480480
}
481481

482+
func Test_GetFileContents_IFC_InsidersMode(t *testing.T) {
483+
t.Parallel()
484+
485+
serverTool := GetFileContents(translations.NullTranslationHelper)
486+
487+
mockRawContent := []byte("hello")
488+
489+
makeMockClient := func(isPrivate bool) *http.Client {
490+
return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
491+
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
492+
GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{
493+
"name": "repo",
494+
"default_branch": "main",
495+
"private": isPrivate,
496+
}),
497+
GetReposCollaboratorsByOwnerByRepo: mockResponse(t, http.StatusOK, []*github.User{
498+
{Login: github.Ptr("octocat")},
499+
{Login: github.Ptr("alice")},
500+
}),
501+
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
502+
w.WriteHeader(http.StatusOK)
503+
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
504+
fileContent := &github.RepositoryContent{
505+
Name: github.Ptr("README.md"),
506+
Path: github.Ptr("README.md"),
507+
SHA: github.Ptr("abc123"),
508+
Type: github.Ptr("file"),
509+
Content: github.Ptr(encodedContent),
510+
Size: github.Ptr(len(mockRawContent)),
511+
Encoding: github.Ptr("base64"),
512+
}
513+
contentBytes, _ := json.Marshal(fileContent)
514+
_, _ = w.Write(contentBytes)
515+
},
516+
})
517+
}
518+
519+
reqParams := map[string]any{
520+
"owner": "octocat",
521+
"repo": "repo",
522+
"path": "README.md",
523+
"ref": "refs/heads/main",
524+
}
525+
526+
t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) {
527+
deps := BaseDeps{
528+
Client: github.NewClient(makeMockClient(false)),
529+
Flags: FeatureFlags{InsidersMode: false},
530+
}
531+
handler := serverTool.Handler(deps)
532+
533+
request := createMCPRequest(reqParams)
534+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
535+
require.NoError(t, err)
536+
require.False(t, result.IsError)
537+
538+
assert.Nil(t, result.Meta, "result meta should be nil when insiders mode is disabled")
539+
})
540+
541+
t.Run("insiders mode enabled on public repo emits public untrusted label", func(t *testing.T) {
542+
deps := BaseDeps{
543+
Client: github.NewClient(makeMockClient(false)),
544+
Flags: FeatureFlags{InsidersMode: true},
545+
}
546+
handler := serverTool.Handler(deps)
547+
548+
request := createMCPRequest(reqParams)
549+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
550+
require.NoError(t, err)
551+
require.False(t, result.IsError)
552+
553+
require.NotNil(t, result.Meta)
554+
ifcLabel, ok := result.Meta["ifc"]
555+
require.True(t, ok, "result meta should contain ifc key")
556+
557+
ifcJSON, err := json.Marshal(ifcLabel)
558+
require.NoError(t, err)
559+
var ifcMap map[string]any
560+
require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap))
561+
562+
assert.Equal(t, "untrusted", ifcMap["integrity"])
563+
confList, ok := ifcMap["confidentiality"].([]any)
564+
require.True(t, ok, "confidentiality should be a list")
565+
require.Len(t, confList, 1)
566+
assert.Equal(t, "public", confList[0])
567+
})
568+
569+
t.Run("insiders mode enabled on private repo emits private trusted label", func(t *testing.T) {
570+
deps := BaseDeps{
571+
Client: github.NewClient(makeMockClient(true)),
572+
Flags: FeatureFlags{InsidersMode: true},
573+
}
574+
handler := serverTool.Handler(deps)
575+
576+
request := createMCPRequest(reqParams)
577+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
578+
require.NoError(t, err)
579+
require.False(t, result.IsError)
580+
581+
require.NotNil(t, result.Meta)
582+
ifcLabel, ok := result.Meta["ifc"]
583+
require.True(t, ok, "result meta should contain ifc key")
584+
585+
ifcJSON, err := json.Marshal(ifcLabel)
586+
require.NoError(t, err)
587+
var ifcMap map[string]any
588+
require.NoError(t, json.Unmarshal(ifcJSON, &ifcMap))
589+
590+
assert.Equal(t, "trusted", ifcMap["integrity"])
591+
confList, ok := ifcMap["confidentiality"].([]any)
592+
require.True(t, ok, "confidentiality should be a list")
593+
assert.Equal(t, []any{"octocat", "alice"}, confList)
594+
})
595+
596+
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
597+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
598+
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
599+
GetReposByOwnerByRepo: mockResponse(t, http.StatusInternalServerError, "boom"),
600+
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
601+
w.WriteHeader(http.StatusOK)
602+
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
603+
fileContent := &github.RepositoryContent{
604+
Name: github.Ptr("README.md"),
605+
Path: github.Ptr("README.md"),
606+
SHA: github.Ptr("abc123"),
607+
Type: github.Ptr("file"),
608+
Content: github.Ptr(encodedContent),
609+
Size: github.Ptr(len(mockRawContent)),
610+
Encoding: github.Ptr("base64"),
611+
}
612+
contentBytes, _ := json.Marshal(fileContent)
613+
_, _ = w.Write(contentBytes)
614+
},
615+
})
616+
deps := BaseDeps{
617+
Client: github.NewClient(mockedClient),
618+
Flags: FeatureFlags{InsidersMode: true},
619+
}
620+
handler := serverTool.Handler(deps)
621+
622+
request := createMCPRequest(reqParams)
623+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
624+
require.NoError(t, err)
625+
require.False(t, result.IsError, "tool call should still succeed when visibility lookup fails")
626+
627+
if result.Meta != nil {
628+
_, hasIFC := result.Meta["ifc"]
629+
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
630+
}
631+
})
632+
}
633+
482634
func Test_ForkRepository(t *testing.T) {
483635
// Verify tool definition once
484636
serverTool := ForkRepository(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)