Improvements & refactoring of get_file_contents (#1582)

* Improvements & refactoring of get_file_contents

* Fix logical path when file or directory not found

* Fix comment

* Docs update

* Do file matching when raw API returns error

Author: almaleksia

README.md

@@ -1092,7 +1092,7 @@ The following sets of tools are available:
 
 - **get_file_contents** - Get file or directory contents
   - `owner`: Repository owner (username or organization) (string, required)
-  - `path`: Path to file/directory (directories must end with a slash '/') (string, optional)
+  - `path`: Path to file/directory (string, optional)
   - `ref`: Accepts optional git refs such as `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head` (string, optional)
   - `repo`: Repository name (string, required)
   - `sha`: Accepts optional commit SHA. If specified, it will be used instead of ref (string, optional)

pkg/github/__toolsnaps__/get_file_contents.snap

@@ -17,7 +17,7 @@
       },
       "path": {
         "type": "string",
-        "description": "Path to file/directory (directories must end with a slash '/')",
+        "description": "Path to file/directory",
         "default": "/"
       },
       "ref": {

pkg/github/repositories.go

@@ -560,7 +560,7 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t
 				},
 				"path": {
 					Type:        "string",
-					Description: "Path to file/directory (directories must end with a slash '/')",
+					Description: "Path to file/directory",
 					Default:     json.RawMessage(`"/"`),
 				},
 				"ref": {
@@ -608,28 +608,26 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t
 			return utils.NewToolResultError(fmt.Sprintf("failed to resolve git reference: %s", err)), nil, nil
 		}
 
-		// If the path is (most likely) not to be a directory, we will
-		// first try to get the raw content from the GitHub raw content API.
+		if rawOpts.SHA != "" {
+			ref = rawOpts.SHA
+		}
 
-		var rawAPIResponseCode int
-		if path != "" && !strings.HasSuffix(path, "/") {
-			// First, get file info from Contents API to retrieve SHA
-			var fileSHA string
-			opts := &github.RepositoryContentGetOptions{Ref: ref}
-			fileContent, _, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
-			if respContents != nil {
-				defer func() { _ = respContents.Body.Close() }()
-			}
-			if err != nil {
-				return ghErrors.NewGitHubAPIErrorResponse(ctx,
-					"failed to get file SHA",
-					respContents,
-					err,
-				), nil, nil
-			}
-			if fileContent == nil || fileContent.SHA == nil {
-				return utils.NewToolResultError("file content SHA is nil, if a directory was requested, path parameters should end with a trailing slash '/'"), nil, nil
-			}
+		var fileSHA string
+		opts := &github.RepositoryContentGetOptions{Ref: ref}
+
+		// Always call GitHub Contents API first to get metadata including SHA and determine if it's a file or directory
+		fileContent, dirContent, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
+		if respContents != nil {
+			defer func() { _ = respContents.Body.Close() }()
+		}
+
+		// The path does not point to a file or directory.
+		// Instead let's try to find it in the Git Tree by matching the end of the path.
+		if err != nil || (fileContent == nil && dirContent == nil) {
+			return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, 0)
+		}
+
+		if fileContent != nil && fileContent.SHA != nil {
 			fileSHA = *fileContent.SHA
 
 			rawClient, err := getRawClient(ctx)
@@ -702,55 +700,19 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t
 				}
 				return utils.NewToolResultResource("successfully downloaded binary file", result), nil, nil
 			}
-			rawAPIResponseCode = resp.StatusCode
-		}
 
-		if rawOpts.SHA != "" {
-			ref = rawOpts.SHA
-		}
-		if strings.HasSuffix(path, "/") {
-			opts := &github.RepositoryContentGetOptions{Ref: ref}
-			_, dirContent, resp, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
-			if err == nil && resp.StatusCode == http.StatusOK {
-				defer func() { _ = resp.Body.Close() }()
-				r, err := json.Marshal(dirContent)
-				if err != nil {
-					return utils.NewToolResultError("failed to marshal response"), nil, nil
-				}
-				return utils.NewToolResultText(string(r)), nil, nil
-			}
-		}
-
-		// The path does not point to a file or directory.
-		// Instead let's try to find it in the Git Tree by matching the end of the path.
-
-		// Step 1: Get Git Tree recursively
-		tree, resp, err := client.Git.GetTree(ctx, owner, repo, ref, true)
-		if err != nil {
-			return ghErrors.NewGitHubAPIErrorResponse(ctx,
-				"failed to get git tree",
-				resp,
-				err,
-			), nil, nil
-		}
-		defer func() { _ = resp.Body.Close() }()
-
-		// Step 2: Filter tree for matching paths
-		const maxMatchingFiles = 3
-		matchingFiles := filterPaths(tree.Entries, path, maxMatchingFiles)
-		if len(matchingFiles) > 0 {
-			matchingFilesJSON, err := json.Marshal(matchingFiles)
+			// Raw API call failed
+			return matchFiles(ctx, client, owner, repo, ref, path, rawOpts, resp.StatusCode)
+		} else if dirContent != nil {
+			// file content or file SHA is nil which means it's a directory
+			r, err := json.Marshal(dirContent)
 			if err != nil {
-				return utils.NewToolResultError(fmt.Sprintf("failed to marshal matching files: %s", err)), nil, nil
+				return utils.NewToolResultError("failed to marshal response"), nil, nil
 			}
-			resolvedRefs, err := json.Marshal(rawOpts)
-			if err != nil {
-				return utils.NewToolResultError(fmt.Sprintf("failed to marshal resolved refs: %s", err)), nil, nil
-			}
-			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
+			return utils.NewToolResultText(string(r)), nil, nil
 		}
 
-		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
+		return utils.NewToolResultError("failed to get file contents"), nil, nil
 	})
 
 	return tool, handler
@@ -2115,3 +2077,35 @@ func UnstarRepository(getClient GetClientFn, t translations.TranslationHelperFun
 
 	return tool, handler
 }
+
+func matchFiles(ctx context.Context, client *github.Client, owner, repo, ref, path string, rawOpts *raw.ContentOpts, rawAPIResponseCode int) (*mcp.CallToolResult, any, error) {
+	// Step 1: Get Git Tree recursively
+	tree, response, err := client.Git.GetTree(ctx, owner, repo, ref, true)
+	if err != nil {
+		return ghErrors.NewGitHubAPIErrorResponse(ctx,
+			"failed to get git tree",
+			response,
+			err,
+		), nil, nil
+	}
+	defer func() { _ = response.Body.Close() }()
+
+	// Step 2: Filter tree for matching paths
+	const maxMatchingFiles = 3
+	matchingFiles := filterPaths(tree.Entries, path, maxMatchingFiles)
+	if len(matchingFiles) > 0 {
+		matchingFilesJSON, err := json.Marshal(matchingFiles)
+		if err != nil {
+			return utils.NewToolResultError(fmt.Sprintf("failed to marshal matching files: %s", err)), nil, nil
+		}
+		resolvedRefs, err := json.Marshal(rawOpts)
+		if err != nil {
+			return utils.NewToolResultError(fmt.Sprintf("failed to marshal resolved refs: %s", err)), nil, nil
+		}
+		if rawAPIResponseCode > 0 {
+			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
+		}
+		return utils.NewToolResultText(fmt.Sprintf("Resolved potential matches in the repository tree (resolved refs: %s, matching files: %s).", string(resolvedRefs), string(matchingFilesJSON))), nil, nil
+	}
+	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
+}