- Notifications
You must be signed in to change notification settings - Fork3.3k
Fallback to default branch in get_file_contents when main doesn't exist#1669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR implements automatic fallback to the default branch when the LLM passes "main" as the ref parameter toget_file_contents and that branch doesn't exist. The change helps avoid unnecessary failures and reduces round-trips to the LLM by auto-resolving to the repository's default branch. A note is added to the success message to inform the user that the ref was changed.
Key changes:
- Added
resolveDefaultBranchhelper function to fetch the repository's default branch reference - Modified
resolveGitReferenceto fallback to the default branch when "main" doesn't exist - Updated success messages in
GetFileContentsto include a note when fallback occurs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/github/repositories.go | AddedresolveDefaultBranch function and fallback logic inresolveGitReference; added success note when ref is changed |
| pkg/github/repositories_test.go | Added test case to verify fallback behavior and success note message |
| deferfunc() {_=resp.Body.Close() }() | ||
| iferr!=nil { | ||
| _,_=ghErrors.NewGitHubAPIErrorToCtx(ctx,"failed to get default branch reference",resp,err) | ||
| returnnil,fmt.Errorf("failed to get default branch reference: %w",err) | ||
| } |
CopilotAIDec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The defer statement to close the response body is placed before the error check. If the API call fails and returns a nil response, this will cause a nil pointer dereference panic when the deferred function executes. The defer should be placed after the error check, following the pattern used elsewhere in this file (e.g., line 1158).
| deferfunc() {_=resp.Body.Close() }() | |
| iferr!=nil { | |
| _,_=ghErrors.NewGitHubAPIErrorToCtx(ctx,"failed to get default branch reference",resp,err) | |
| returnnil,fmt.Errorf("failed to get default branch reference: %w",err) | |
| } | |
| iferr!=nil { | |
| _,_=ghErrors.NewGitHubAPIErrorToCtx(ctx,"failed to get default branch reference",resp,err) | |
| returnnil,fmt.Errorf("failed to get default branch reference: %w",err) | |
| } | |
| deferfunc() {_=resp.Body.Close() }() |
| iferr!=nil { | ||
| _,_=ghErrors.NewGitHubAPIErrorToCtx(ctx,"failed to get repository info",resp,err) | ||
| returnnil,fmt.Errorf("failed to get repository info: %w",err) | ||
| } |
CopilotAIDec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The response body from the Repositories.Get call is not being closed, which could lead to a resource leak. Add a defer statement to close resp.Body after the error check, similar to the pattern at line 1158 in this file.
| } | |
| } | |
| deferfunc() {_=resp.Body.Close() }() |
| if!strings.HasSuffix(rawOpts.Ref,originalRef) { | ||
| successNote=fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.",originalRef,rawOpts.Ref) |
CopilotAIDec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The logic to determine if a fallback occurred uses HasSuffix, which could produce false negatives. If the default branch name happens to end with the original ref value (e.g., originalRef is "main" and the default branch is "whatevermain"), the note won't be shown even though a fallback occurred. Consider using a more precise check, such as comparing if rawOpts.Ref equals the expected ref format, or store a boolean flag during resolution to indicate when a fallback happens.
| if!strings.HasSuffix(rawOpts.Ref,originalRef) { | |
| successNote=fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.",originalRef,rawOpts.Ref) | |
| iforiginalRef!=""&&rawOpts.Ref!="" { | |
| sameAsOriginal:=rawOpts.Ref==originalRef||rawOpts.Ref==fmt.Sprintf("refs/heads/%s",originalRef) | |
| if!sameAsOriginal { | |
| successNote=fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.",originalRef,rawOpts.Ref) | |
| } |
tommaso-moro left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great improvement! I love the idea of falling back to the default branch whenmain doesn't exist.
The only concern I have here is about maintainability. Specifically, here:
if !strings.HasSuffix(rawOpts.Ref, originalRef) {successNote = fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.", originalRef, rawOpts.Ref)}We are assuming that if!strings.HasSuffix(rawOpts.Ref, originalRef) is true then itmust be due to the fact that we used the fallback logic. This might work today because we know the only case where the resolved ref wouldn't end with the original ref is the fallback, but it introduces implicit coupling: for example, if we ever changeresolveGitReference to transform refs in other ways (e.g. lowercasing) the suffix check could fail even when no fallback was used.
And I think we might run into weird edge cases too with the current logic: for example if a user passesmain and the default branch issomething-main the fallback would be used, but no note would be generated even though a fallback occurred.
Given this I wonder if we should make the logic here more explicit. For example theresolveGitReference function could return an explicit signal when a fallback has been used. That way we could do an explicit check likeif result.FallbackOccurred orif result.FallbackRef != "" rather than inferring the fallback from the string comparisons.
What do you think?
| // main branch ref passed in ref parameter but it doesn't exist - default branch was used | ||
| varsuccessNotestring | ||
| if!strings.HasSuffix(rawOpts.Ref,originalRef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To me this seems like a fragile check, it doesn't account for edge cases (e.g. if the user passesmain and the default branch is calledsomething-main) and I am worried that if we changeresolveGitReference we need to update this check here too as it seems quite coupled to the internal implementation ofresolveGitReference. Wdyt?
Summary
Follow up for#1655
When LLM passes
mainas ref toget_file_contentsand it doesn't exist - auto-resolve default repo branch instead.Why
When main branch is passed we assume the intent to get file content from the default branch to avoid unnecessary failure and as a result - unnecessary requests to LLM.
The note that branch was changed is added in the success message.
MCP impact
Prompts tested (tool changes only)
Security / limits
Lint & tests
./script/lint./script/testDocs