Fix SHA validation in create_or_update_file#2134
Conversation
| minimalResponse := convertToMinimalFileContentResponse(fileContent) | ||
|
|
||
| // Warn if file was updated without SHA validation (blind update) | ||
| if sha == "" && previousSHA != "" { |
There was a problem hiding this comment.
removing blind update completely
There was a problem hiding this comment.
I think it's fair to accept the errors, thought this is probably the most controversial change.
There was a problem hiding this comment.
I decided if GitHub API requires sha for updates who are we to make it optional :D
There was a problem hiding this comment.
Well, I think it's fine to make agent fail, but I don't think it's wrong either way, just different trade-offs.
You still retain a git record of what you changed, so it might be surprising to update a file that has changed since you last checked, but you can at least still work out what happened.
The concern for me was always concurrent updates from different agents and then thinking independently that two things were done, but actually only one was because the other overwrites it. That's not great, and I think it's ok for us to not allow that by accident.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect SHA validation in the create_or_update_file tool by switching from HEAD/ETag-based checks to using the GitHub Contents API’s returned blob SHA, and updates the tool’s behavior/docs accordingly.
Changes:
- Replace conditional HEAD/ETag SHA validation with Contents API metadata (
.sha) lookups. - Require
shawhen updating an existing file (reject blind overwrites). - Update unit tests and tool schema snapshot to reflect the new behavior and guidance.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/github/repositories.go |
Reworks create_or_update_file SHA validation/existence checks and updates user-facing guidance. |
pkg/github/repositories_test.go |
Updates test cases to mock Contents API GET behavior and assert new validation outcomes. |
pkg/github/__toolsnaps__/create_or_update_file.snap |
Updates tool description/schema snapshot to match new guidance and behavior. |
SamMorrowDrums
left a comment
There was a problem hiding this comment.
All makes sense.
Summary
We incorrectly return SHA mismatch for correct updates.
Changes:
Why
Fixes #2133
What changed
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs