Skip to content

fix(repos): give create_or_update_file callers a SHA they can actually get #3131

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SamMorrowDrums merged 4 commits into from
Sep 1, 2026
Merged

fix(repos): give create_or_update_file callers a SHA they can actually get #3131

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2212c0
fix(repos): give create_or_update_file callers a SHA they can actuall…
dylanpulver Aug 20, 2026
4e9f519
fix(repos): stop disclosing the blob SHA when no sha was supplied
dylanpulver Aug 21, 2026
2c8d480
Merge branch 'main' into fix/create-or-update-file-sha-guidance
SamMorrowDrums Aug 30, 2026
ee97d02
fix(repos): make SHA recovery guidance actionable
SamMorrowDrums Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(repos): stop disclosing the blob SHA when no sha was supplied 
The already-exists path is reached only when the caller sent no sha, so
it has not read the file. Returning the current blob SHA there let it
overwrite content it never saw on the next call, which is the race the
SHA gate exists to prevent. Send the caller to get_file_contents for the
path and ref instead, so obtaining the SHA still requires reading the
file.

The stale-SHA path is unaffected: the caller did supply a sha, and that
message already reported the current SHA before this change. Only its
recovery step moved off git rev-parse, and it is now imperative rather
than conditional, since a stale SHA means the file definitely changed.

Assert the already-exists error does not contain the blob SHA so the
gate cannot be loosened again without a test failing.
  • Loading branch information
@dylanpulver
dylanpulver committed Aug 21, 2026
commit 4e9f5194a5abeb99c5a92e2008ff41a1b3a2381a
10 changes: 6 additions & 4 deletions pkg/github/repositories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,9 @@ SHA MUST be provided for existing file updates.
if currentSHA != sha {
return utils.NewToolResultError(fmt.Sprintf(
"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
"Re-read the file with get_file_contents if you need its latest content, then retry with the sha parameter set to %s.",
sha, currentSHA, currentSHA)), nil, nil
"The file changed since you read it, so re-read it with get_file_contents for this path and ref, "+
"rebuild your content against what it returns, and retry with the sha parameter set to the SHA that call reports.",
sha, currentSHA)), nil, nil
}
if !allowSymlinkWrite {
if existingFile.GetType() == "symlink" {
Expand Down Expand Up @@ -593,8 +594,9 @@ SHA MUST be provided for existing file updates.
// File exists but no SHA was provided - reject to prevent blind overwrites
return utils.NewToolResultError(fmt.Sprintf(
"File already exists at %s. You must provide the current file's SHA when updating. "+
"The current SHA is %s; retry with the sha parameter set to that value.",
path, existingFile.GetSHA())), nil, nil
"Call get_file_contents for this path and ref to read the file you are about to overwrite, "+
"then retry with the sha parameter set to the blob SHA that call reports.",
path)), nil, nil
}
// If file not found, no previous SHA needed (new file creation)
}
Expand Down
13 changes: 11 additions & 2 deletions pkg/github/repositories_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2138,6 +2138,7 @@ func Test_CreateOrUpdateFile(t *testing.T) {
expectedContent *github.RepositoryContentResponse
expectedErrMsg string
expectedErrMsgs []string
unexpectedErrMsgs []string
expectedRequestCount int
}{
{
Expand Down Expand Up @@ -2473,7 +2474,8 @@ func Test_CreateOrUpdateFile(t *testing.T) {
expectError: true,
expectedErrMsgs: []string{
"SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888",
"retry with the sha parameter set to newsha999888",
"re-read it with get_file_contents",
"retry with the sha parameter set to the SHA that call reports",
},
expectedRequestCount: 1,
},
Expand Down Expand Up @@ -2539,8 +2541,12 @@ func Test_CreateOrUpdateFile(t *testing.T) {
expectError: true,
expectedErrMsgs: []string{
"File already exists at docs/example.md",
"The current SHA is existing123; retry with the sha parameter set to that value.",
"Call get_file_contents for this path and ref",
"retry with the sha parameter set to the blob SHA that call reports",
},
// A caller that never supplied a SHA has not read this file, so the
// error must not hand it one to overwrite with.
unexpectedErrMsgs: []string{"existing123"},
expectedRequestCount: 1,
},
{
Expand Down Expand Up @@ -2615,6 +2621,9 @@ func Test_CreateOrUpdateFile(t *testing.T) {
for _, expectedErrMsg := range tc.expectedErrMsgs {
assert.Contains(t, errorText.String(), expectedErrMsg)
}
for _, unexpectedErrMsg := range tc.unexpectedErrMsgs {
assert.NotContains(t, errorText.String(), unexpectedErrMsg)
}
// The caller of this tool works over the API and has no working
// tree, so errors must never ask it to run a local git command.
assert.NotContains(t, errorText.String(), "git rev-parse")
Expand Down
Loading