Tommy/improve-ref-handling #851
Merged
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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR significantly improves the ref handling in the get_file_contents tool to better handle different ref formats that models commonly provide. The issue was that while the GitHub API's content endpoint can handle both fully-qualified refs (refs/heads/branch_name) and short names (branch_name), the reference endpoint requires fully-qualified refs, causing the tool to fail frequently.
Key changes:
- Enhanced
resolveGitReferencefunction with robust logic to handle fully-qualified, partially-qualified, and short name refs - Added comprehensive test coverage for all ref resolution scenarios
- Improved error handling and logging for better diagnostics
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/github/repositories.go | Enhanced resolveGitReference function with improved ref resolution logic and detailed documentation |
| pkg/github/repositories_test.go | Added comprehensive test cases covering all ref format scenarios and error conditions |
almaleksia
reviewed
Aug 11, 2025
Contributor
almaleksia
left a comment
almaleksia
left a comment
There was a problem hiding this comment.
I really like this logic!
Left some comments, mostly flow and readability related.
almaleksia
reviewed
Aug 11, 2025
…f in case definitions and some of the logic
almaleksia
reviewed
Aug 11, 2025
almaleksia
approved these changes
Aug 11, 2025
nickytonline
pushed a commit
to nickytonline/github-mcp-http
that referenced
this pull request
Oct 4, 2025
* make resolveGitReference function more robust, add comments to follow the logic * add tests for new functionality * lint fix 1 * fix linting by using inverted if instead of empty if block * remove unused var * refactor * remove comment * small fix * add ref == "" case in switch statement, use originalRef instead of ref in case definitions and some of the logic
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes: #728
Overview
This PR significantly improves how we handle refs in the
get_file_contentstool, after users reported the tool being often broken from bad ref handling.The problem
Despite the tool description, models can pass as argument to the tool a
refformatted either as fully qualified (e.g.refs/heads/beanch_name) or as short user-friendly names (e.g.branch_name).The get_repository_content endpoint from the Github API is able to handle both flexibly:
However, the
get_referenceendpoint doesn't (it requires arefformatted in a specific way):Our previous logic would fail often because it uses both endpoints but it was not handling user-friendly refs (e.g.
branch_name,heads/branch_name,tag_name), which are often provided by models to the tool. This would thus break the tool.The Solution
This PR introduces new, robust logic to resolve a
refby identifying the format in which it was provided and constructing a fully qualifiedrefaccordingly. In short, the newresolveGitReferencefunction now handles:refs/heads/mainheads/mainmain(discovering automatically if it's a branch or a tag)This ensures that the tool can handle the most common ways in which the
refinput is provided by the user.Additionally, I added:
Demo
