Always include SHA in get_file_contents responses #676
+59
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ub#595) Enhance get_file_contents to include SHA information without changing the existing MCP server response format. Changes: - Add Contents API call to retrieve SHA before fetching raw content - Include SHA in resourceURI (repo://owner/repo/sha/{SHA}/contents/path) - Add SHA to success messages - Update tests to verify SHA inclusion - Maintain original behavior: text files return raw text, binaries return base64 This preserves backward compatibility while providing SHA information for better file versioning support. Closes github#595
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the get_file_contents tool to always include SHA information in responses by adding a preliminary Contents API call before fetching raw content. This provides a simpler, backward-compatible solution that automatically includes SHA data without requiring parameter changes.
Key changes:
- Added Contents API call to retrieve file metadata (including SHA) before fetching raw content
- Updated resource URI format to include SHA instead of branch reference
- Enhanced success messages to include SHA information
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/github/repositories.go | Added Contents API call to retrieve SHA before raw content fetch and updated URI/message formatting |
| pkg/github/repositories_test.go | Updated test mocks to include Contents API responses and adjusted expected URIs to use SHA format |
Comments suppressed due to low confidence (1)
pkg/github/repositories.go:513
- The variable name 'errContents' is inconsistent with Go naming conventions. It should be 'err' or follow the existing pattern used elsewhere in the function.
fileContent, _, respContents, errContents := client.Repositories.GetContents(ctx, owner, repo, path, opts)
yonaka15
changed the title
Ensure response body is properly closed even when an error occurs by moving the defer statement before the error check. This prevents potential resource leaks when the Contents API returns an error with a non-nil response. Changes: - Move defer respContents.Body.Close() before error checking - Rename errContents to err for consistency - Add nil check for respContents before attempting to close body This follows Go best practices for handling HTTP responses and prevents potential goroutine/memory leaks.
yonaka15
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR provides a simpler solution to #595 by always including SHA information in the
get_file_contentstool responses without requiring any parameter changes.Unlike #605 which adds an
include_shaparameter, this approach transparently enhances all responses with SHA information while preserving the existing MCP server behavior.Approach
The implementation adds a preliminary Contents API call to retrieve SHA information before fetching the actual content:
This dual-API approach ensures:
create_or_update_fileoperationsChanges
pkg/github/repositories.goto add Contents API call before Raw Content APIpkg/github/repositories_test.goto include Contents API mocksBenefits over #605
include_sha=trueTesting
Verified with both unit tests and MCP Inspector against real repositories:
Closes #595