Skip to content

Commit 4fc24be

Browse files
committed
get_file_contents: address Copilot review findings
- FetchRepoIsPrivate: tighten doc to 'returns whether a repository is private' and close the underlying *github.Response body. - attachIFC: skip emitting the ifc label when the repository visibility lookup fails, instead of falling through to PublicUntrusted (which would mislabel a private or unknown-visibility repo as public). The failure is no longer cached so a subsequent return path can retry. - Add a test asserting the tool still succeeds and omits result.Meta ["ifc"] when the visibility lookup returns 500.
1 parent 3f1e699 commit 4fc24be

2 files changed

Lines changed: 56 additions & 12 deletions

File tree

pkg/github/repositories.go

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -682,11 +682,14 @@ func FetchRepoCollaborators(ctx context.Context, client *github.Client, owner, r
682682
return logins, nil
683683
}
684684

685-
// FetchRepoIsPrivate returns the visibility of a repository. It is a thin
685+
// FetchRepoIsPrivate returns whether a repository is private. It is a thin
686686
// wrapper around the GitHub Repositories.Get endpoint provided as a shared
687687
// helper for IFC label computation across tools.
688688
func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo string) (bool, error) {
689-
r, _, err := client.Repositories.Get(ctx, owner, repo)
689+
r, resp, err := client.Repositories.Get(ctx, owner, repo)
690+
if resp != nil {
691+
defer func() { _ = resp.Body.Close() }()
692+
}
690693
if err != nil {
691694
return false, err
692695
}
@@ -768,22 +771,26 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
768771
// attachIFC adds the IFC label to a successful tool result when
769772
// InsidersMode is enabled. The visibility and (for private
770773
// repositories) collaborators lookups are performed lazily on
771-
// first use and are best-effort: if they fail we still attach
772-
// the label, falling back to the repository owner so the reader
773-
// set is never empty for a private repo.
774+
// first use. If the visibility lookup fails we skip the label
775+
// rather than misclassify the result; the failure is not cached
776+
// so a later return path can retry. If only the collaborators
777+
// lookup fails for a private repo we fall back to the owner so
778+
// the reader set is never empty.
774779
var (
775-
ifcLabelLoaded bool
776-
ifcIsPrivate bool
777-
ifcReaders []string
780+
ifcLabelKnown bool
781+
ifcIsPrivate bool
782+
ifcReaders []string
778783
)
779784
attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult {
780785
if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode {
781786
return r
782787
}
783-
if !ifcLabelLoaded {
784-
if isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo); err == nil {
785-
ifcIsPrivate = isPrivate
788+
if !ifcLabelKnown {
789+
isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo)
790+
if err != nil {
791+
return r
786792
}
793+
ifcIsPrivate = isPrivate
787794
if ifcIsPrivate {
788795
if collaborators, err := FetchRepoCollaborators(ctx, client, owner, repo); err == nil {
789796
ifcReaders = collaborators
@@ -792,7 +799,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
792799
ifcReaders = []string{owner}
793800
}
794801
}
795-
ifcLabelLoaded = true
802+
ifcLabelKnown = true
796803
}
797804
if r.Meta == nil {
798805
r.Meta = mcp.Meta{}

pkg/github/repositories_test.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,43 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) {
590590
require.True(t, ok, "confidentiality should be a list")
591591
assert.Equal(t, []any{"octocat", "alice"}, confList)
592592
})
593+
594+
t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) {
595+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
596+
GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"),
597+
GetReposByOwnerByRepo: mockResponse(t, http.StatusInternalServerError, "boom"),
598+
GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) {
599+
w.WriteHeader(http.StatusOK)
600+
encodedContent := base64.StdEncoding.EncodeToString(mockRawContent)
601+
fileContent := &github.RepositoryContent{
602+
Name: github.Ptr("README.md"),
603+
Path: github.Ptr("README.md"),
604+
SHA: github.Ptr("abc123"),
605+
Type: github.Ptr("file"),
606+
Content: github.Ptr(encodedContent),
607+
Size: github.Ptr(len(mockRawContent)),
608+
Encoding: github.Ptr("base64"),
609+
}
610+
contentBytes, _ := json.Marshal(fileContent)
611+
_, _ = w.Write(contentBytes)
612+
},
613+
})
614+
deps := BaseDeps{
615+
Client: github.NewClient(mockedClient),
616+
Flags: FeatureFlags{InsidersMode: true},
617+
}
618+
handler := serverTool.Handler(deps)
619+
620+
request := createMCPRequest(reqParams)
621+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
622+
require.NoError(t, err)
623+
require.False(t, result.IsError, "tool call should still succeed when visibility lookup fails")
624+
625+
if result.Meta != nil {
626+
_, hasIFC := result.Meta["ifc"]
627+
assert.False(t, hasIFC, "ifc label should be omitted when visibility lookup fails")
628+
}
629+
})
593630
}
594631

595632
func Test_ForkRepository(t *testing.T) {

0 commit comments

Comments
 (0)