Skip to content

Commit fa18a2f

Browse files
SamMorrowDrumsSamMorrowDrums
committed
fix(get_file_contents): Restore correct implementation from #1582
The refactor incorrectly restructured the GetFileContents logic: - Move 'if rawOpts.SHA != "" { ref = rawOpts.SHA }' before GetContents call - Always call GetContents first (not conditionally based on path suffix) - Restore matchFiles helper function for proper fallback handling - Use matchFiles when Contents API fails or raw API fails This aligns with the improvements from PR #1582 that was merged into main.
1 parent b923529 commit fa18a2f

File tree

1 file changed

+113
-109
lines changed

1 file changed

+113
-109
lines changed

‎pkg/github/repositories.go‎

Lines changed: 113 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -693,141 +693,111 @@ func GetFileContents(t translations.TranslationHelperFunc) registry.ServerTool {
693693
return utils.NewToolResultError(fmt.Sprintf("failed to resolve git reference: %s", err)), nil, nil
694694
}
695695

696-
// If the path is (most likely) not to be a directory, we will
697-
// first try to get the raw content from the GitHub raw content API.
698-
699-
var rawAPIResponseCode int
700-
if path != "" && !strings.HasSuffix(path, "/") {
701-
// First, get file info from Contents API to retrieve SHA
702-
var fileSHA string
703-
opts := &github.RepositoryContentGetOptions{Ref: ref}
704-
fileContent, _, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
705-
if respContents != nil {
706-
defer func() { _ = respContents.Body.Close() }()
696+
if rawOpts.SHA != "" {
697+
ref = rawOpts.SHA
698+
}
699+
700+
var fileSHA string
701+
opts := &github.RepositoryContentGetOptions{Ref: ref}
702+
703+
// Always call GitHub Contents API first to get metadata including SHA and determine if it's a file or directory
704+
fileContent, dirContent, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
705+
if respContents != nil {
706+
defer func() { _ = respContents.Body.Close() }()
707+
}
708+
709+
// The path does not point to a file or directory.
710+
// Instead let's try to find it in the Git Tree by matching the end of the path.
711+
if err != nil || (fileContent == nil && dirContent == nil) {
712+
return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
713+
}
714+
715+
if fileContent != nil && fileContent.SHA != nil {
716+
fileSHA = *fileContent.SHA
717+
718+
rawClient, err := deps.GetRawClient(ctx)
719+
if err != nil {
720+
return utils.NewToolResultError("failed to get GitHub raw content client"), nil, nil
707721
}
708-
if err == nil && fileContent != nil && fileContent.SHA != nil {
709-
fileSHA = *fileContent.SHA
722+
resp, err := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts)
723+
if err != nil {
724+
return utils.NewToolResultError("failed to get raw repository content"), nil, nil
725+
}
726+
defer func() {
727+
_ = resp.Body.Close()
728+
}()
710729

711-
rawClient, err := deps.GetRawClient(ctx)
712-
if err != nil {
713-
return utils.NewToolResultError("failed to get GitHub raw content client"), nil, nil
714-
}
715-
resp, err := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts)
730+
if resp.StatusCode == http.StatusOK {
731+
// If the raw content is found, return it directly
732+
body, err := io.ReadAll(resp.Body)
716733
if err != nil {
717-
return utils.NewToolResultError("failed to get raw repository content"), nil, nil
734+
return utils.NewToolResultError("failed to read response body"), nil, nil
718735
}
719-
defer func() {
720-
_ = resp.Body.Close()
721-
}()
736+
contentType := resp.Header.Get("Content-Type")
722737

723-
if resp.StatusCode == http.StatusOK {
724-
// If the raw content is found, return it directly
725-
body, err := io.ReadAll(resp.Body)
738+
var resourceURI string
739+
switch {
740+
case sha != "":
741+
resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path)
726742
if err != nil {
727-
return utils.NewToolResultError("failed to read response body"), nil, nil
743+
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
728744
}
729-
contentType := resp.Header.Get("Content-Type")
730-
731-
var resourceURI string
732-
switch {
733-
case sha != "":
734-
resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path)
735-
if err != nil {
736-
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
737-
}
738-
case ref != "":
739-
resourceURI, err = url.JoinPath("repo://", owner, repo, ref, "contents", path)
740-
if err != nil {
741-
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
742-
}
743-
default:
744-
resourceURI, err = url.JoinPath("repo://", owner, repo, "contents", path)
745-
if err != nil {
746-
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
747-
}
745+
case ref != "":
746+
resourceURI, err = url.JoinPath("repo://", owner, repo, ref, "contents", path)
747+
if err != nil {
748+
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
748749
}
749-
750-
// Determine if content is text or binary
751-
isTextContent := strings.HasPrefix(contentType, "text/") ||
752-
contentType == "application/json" ||
753-
contentType == "application/xml" ||
754-
strings.HasSuffix(contentType, "+json") ||
755-
strings.HasSuffix(contentType, "+xml")
756-
757-
if isTextContent {
758-
result := &mcp.ResourceContents{
759-
URI: resourceURI,
760-
Text: string(body),
761-
MIMEType: contentType,
762-
}
763-
// Include SHA in the result metadata
764-
if fileSHA != "" {
765-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)", fileSHA), result), nil, nil
766-
}
767-
return utils.NewToolResultResource("successfully downloaded text file", result), nil, nil
750+
default:
751+
resourceURI, err = url.JoinPath("repo://", owner, repo, "contents", path)
752+
if err != nil {
753+
return nil, nil, fmt.Errorf("failed to create resource URI: %w", err)
768754
}
755+
}
769756

757+
// Determine if content is text or binary
758+
isTextContent := strings.HasPrefix(contentType, "text/") ||
759+
contentType == "application/json" ||
760+
contentType == "application/xml" ||
761+
strings.HasSuffix(contentType, "+json") ||
762+
strings.HasSuffix(contentType, "+xml")
763+
764+
if isTextContent {
770765
result := &mcp.ResourceContents{
771766
URI: resourceURI,
772-
Blob: body,
767+
Text: string(body),
773768
MIMEType: contentType,
774769
}
775770
// Include SHA in the result metadata
776771
if fileSHA != "" {
777-
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)", fileSHA), result), nil, nil
772+
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)", fileSHA), result), nil, nil
778773
}
779-
return utils.NewToolResultResource("successfully downloaded binary file", result), nil, nil
774+
return utils.NewToolResultResource("successfully downloaded text file", result), nil, nil
780775
}
781-
rawAPIResponseCode = resp.StatusCode
782-
}
783-
}
784776

785-
if rawOpts.SHA != "" {
786-
ref = rawOpts.SHA
787-
}
788-
if strings.HasSuffix(path, "/") {
789-
opts := &github.RepositoryContentGetOptions{Ref: ref}
790-
_, dirContent, resp, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
791-
if err == nil && resp.StatusCode == http.StatusOK {
792-
defer func() { _ = resp.Body.Close() }()
793-
r, err := json.Marshal(dirContent)
794-
if err != nil {
795-
return utils.NewToolResultError("failed to marshal response"), nil, nil
777+
result := &mcp.ResourceContents{
778+
URI: resourceURI,
779+
Blob: body,
780+
MIMEType: contentType,
781+
}
782+
// Include SHA in the result metadata
783+
if fileSHA != "" {
784+
return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)", fileSHA), result), nil, nil
796785
}
797-
return utils.NewToolResultText(string(r)), nil, nil
786+
return utils.NewToolResultResource("successfully downloaded binary file", result), nil, nil
798787
}
799-
}
800-
801-
// The path does not point to a file or directory.
802-
// Instead let's try to find it in the Git Tree by matching the end of the path.
803-
804-
// Step 1: Get Git Tree recursively
805-
tree, resp, err := client.Git.GetTree(ctx, owner, repo, ref, true)
806-
if err != nil {
807-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
808-
"failed to get git tree",
809-
resp,
810-
err,
811-
), nil, nil
812-
}
813-
defer func() { _ = resp.Body.Close() }()
814788

815-
// Step 2: Filter tree for matching paths
816-
const maxMatchingFiles = 3
817-
matchingFiles := filterPaths(tree.Entries, path, maxMatchingFiles)
818-
if len(matchingFiles) > 0 {
819-
matchingFilesJSON, err := json.Marshal(matchingFiles)
820-
if err != nil {
821-
return utils.NewToolResultError(fmt.Sprintf("failed to marshal matching files: %s", err)), nil, nil
822-
}
823-
resolvedRefs, err := json.Marshal(rawOpts)
789+
// Raw API call failed
790+
return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, resp.StatusCode)
791+
} else if dirContent != nil {
792+
// file content or file SHA is nil which means it's a directory
793+
r, err := json.Marshal(dirContent)
824794
if err != nil {
825-
return utils.NewToolResultError(fmt.Sprintf("failed to marshal resolved refs: %s", err)), nil, nil
795+
return utils.NewToolResultError("failed to marshal response"), nil, nil
826796
}
827-
return utils.NewToolResultError(fmt.Sprintf("Resolved potential matches in the repository tree (resolved refs: %s, matching files: %s), but the raw content API returned an unexpected status code %d.", string(resolvedRefs), string(matchingFilesJSON), rawAPIResponseCode)), nil, nil
797+
return utils.NewToolResultText(string(r)), nil, nil
828798
}
829799

830-
return utils.NewToolResultError("Failed to get file contents. The path does not point to a file or directory, or the file does not exist in the repository."), nil, nil
800+
return utils.NewToolResultError("failed to get file contents"), nil, nil
831801
}
832802
},
833803
)
@@ -1817,6 +1787,40 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) registry.ServerTool {
18171787
)
18181788
}
18191789

1790+
// matchFiles searches for files in the Git tree that match the given path.
1791+
// It's used when GetContents fails or returns unexpected results.
1792+
func matchFiles(ctx context.Context, client *github.Client, owner, repo, ref, path string, rawOpts *raw.ContentOpts, rawAPIResponseCode int) (*mcp.CallToolResult, any, error) {
1793+
// Step 1: Get Git Tree recursively
1794+
tree, response, err := client.Git.GetTree(ctx, owner, repo, ref, true)
1795+
if err != nil {
1796+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1797+
"failed to get git tree",
1798+
response,
1799+
err,
1800+
), nil, nil
1801+
}
1802+
defer func() { _ = response.Body.Close() }()
1803+
1804+
// Step 2: Filter tree for matching paths
1805+
const maxMatchingFiles = 3
1806+
matchingFiles := filterPaths(tree.Entries, path, maxMatchingFiles)
1807+
if len(matchingFiles) > 0 {
1808+
matchingFilesJSON, err := json.Marshal(matchingFiles)
1809+
if err != nil {
1810+
return utils.NewToolResultError(fmt.Sprintf("failed to marshal matching files: %s", err)), nil, nil
1811+
}
1812+
resolvedRefs, err := json.Marshal(rawOpts)
1813+
if err != nil {
1814+
return utils.NewToolResultError(fmt.Sprintf("failed to marshal resolved refs: %s", err)), nil, nil
1815+
}
1816+
if rawAPIResponseCode > 0 {
1817+
return utils.NewToolResultText(fmt.Sprintf("Resolved potential matches in the repository tree (resolved refs: %s, matching files: %s), but the content API returned an unexpected status code %d.", string(resolvedRefs), string(matchingFilesJSON), rawAPIResponseCode)), nil, nil
1818+
}
1819+
return utils.NewToolResultText(fmt.Sprintf("Resolved potential matches in the repository tree (resolved refs: %s, matching files: %s).", string(resolvedRefs), string(matchingFilesJSON))), nil, nil
1820+
}
1821+
return utils.NewToolResultError("Failed to get file contents. The path does not point to a file or directory, or the file does not exist in the repository."), nil, nil
1822+
}
1823+
18201824
// filterPaths filters the entries in a GitHub tree to find paths that
18211825
// match the given suffix.
18221826
// maxResults limits the number of results returned to first maxResults entries,

0 commit comments

Comments
 (0)