- Notifications
You must be signed in to change notification settings - Fork899
Fix: Specify sha is required for file update#320
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
Fix: Specify sha is required for file update#320
Uh oh!
There was an error while loading.Please reload this page.
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 updates the tool description for file operations to indicate that a SHA is required when updating a file.
- Updated the tool description for create_or_update_file.
- Clarified that the SHA field must be provided when updating a file.
| @@ -216,7 +216,7 @@ func ListBranches(getClient GetClientFn, t translations.TranslationHelperFunc) ( | |||
| // CreateOrUpdateFile creates a tool to create or update a file in a GitHub repository. | |||
| func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { | |||
| return mcp.NewTool("create_or_update_file", | |||
| mcp.WithDescription(t("TOOL_CREATE_OR_UPDATE_FILE_DESCRIPTION", "Create or update a single file in a GitHub repository")), | |||
| mcp.WithDescription(t("TOOL_CREATE_OR_UPDATE_FILE_DESCRIPTION", "Create or update a single file in a GitHub repository. If updating, you must provide the SHA of the file you want to update.")), | |||
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 updated tool description indicates that a SHA is required when updating a file, but there is no validation enforcing this constraint. Consider adding a check to ensure that the SHA field is provided when processing an update.
Copilot uses AI. Check for mistakes.
juruen commentedApr 28, 2025 • 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.
Thank you so much for the PR. @crondinini Although having separate tools allows us to specify the required field directly in the schema, there have been some concerns about adding too many tools. That’s why we sometimes prefer to merge several tools into one. As far as I know, this isn’t set in stone yet and could definitely change. Having said that, as this is a minor change to the description that might help to make sure that the update operation is called correctly, I think we should merge it. Please, feel free to correct my assumption! I've successfully tested it:
|
@juruen Thank you for the review and for the explanation. That makes sense. Considering this is such a small change that fixes a bug, I agree it makes sense to merge. I had to solve a conflict so the review was automatically dismissed. If you could give your approval again, that would be great! Thank you :) |
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.
Thank you for this 🙏
a7d741c intogithub:mainUh oh!
There was an error while loading.Please reload this page.
Closes:#191
The problem
When the LLM sends an
update_filerequest, it doesn't include theshaof the file being replaced, leading to the error:This happens because
shais an optional field since the same tool is used for bothcreateandupdatefile operations.The solution
I did some local testing and the issue seems to be fixed if we specify in the tool description that
shais required when updating a file.However, a more robust solution might be to split the tool into 2 separate tools and and make a new
update_filetool that has a requiredshaproperty.I wasn't sure which approach would be preferred, so I've decided to put forward the simplest solution that works to hear your feedback.