Skip to content

Conversation

@almaleksia
Copy link
Contributor

Summary

Follow up for #1655
When LLM passes main as ref to get_file_contents and 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

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

Prompts tested (tool changes only)

Security / limits

  • No security or limits impact
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples)

@almaleksia almaleksia requested a review from a team as a code owner December 22, 2025 11:42
Copilot AI review requested due to automatic review settings December 22, 2025 11:42
Copy link
Contributor

Copilot AI left a 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 to get_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 resolveDefaultBranch helper function to fetch the repository's default branch reference
  • Modified resolveGitReference to fallback to the default branch when "main" doesn't exist
  • Updated success messages in GetFileContents to 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 Added resolveDefaultBranch function and fallback logic in resolveGitReference; added success note when ref is changed
pkg/github/repositories_test.go Added test case to verify fallback behavior and success note message

if err != nil {
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository info", resp, err)
return nil, fmt.Errorf("failed to get repository info: %w", err)
}
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
}
}
defer func() { _ = resp.Body.Close() }()

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@tommaso-moro tommaso-moro left a 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 when main 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 it must 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 change resolveGitReference 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 passes main and the default branch is something-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 the resolveGitReference function could return an explicit signal when a fallback has been used. That way we could do an explicit check like if result.FallbackOccurred or if result.FallbackRef != "" rather than inferring the fallback from the string comparisons.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants