Skip to content

Commit 8dad5c2

Browse files
mattdhollowaymattdholloway
authored
Merge branch 'main' into raw-client-error-annotation
2 parents 2b7d781 + afe34d8 commit 8dad5c2

File tree

4 files changed

+260
-6
lines changed

4 files changed

+260
-6
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1061,7 +1061,7 @@ The following sets of tools are available:
10611061
- `owner`: Repository owner (username or organization) (string, required)
10621062
- `path`: Path where to create/update the file (string, required)
10631063
- `repo`: Repository name (string, required)
1064-
- `sha`: Required if updating an existing file. The blob SHA of the file being replaced. (string, optional)
1064+
- `sha`: The blob SHA of the file being replaced. (string, optional)
10651065

10661066
- **create_repository** - Create repository
10671067
- `autoInit`: Initialize with README (boolean, optional)

‎pkg/github/__toolsnaps__/create_or_update_file.snap‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"annotations": {
33
"title": "Create or update file"
44
},
5-
"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. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.",
5+
"description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit ls-tree HEAD \u003cpath to file\u003e\n\nIf the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval.\n",
66
"inputSchema": {
77
"type": "object",
88
"required": [
@@ -40,7 +40,7 @@
4040
},
4141
"sha": {
4242
"type": "string",
43-
"description": "Required if updating an existing file. The blob SHA of the file being replaced."
43+
"description": "The blob SHA of the file being replaced."
4444
}
4545
}
4646
},

‎pkg/github/repositories.go‎

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -310,8 +310,15 @@ func ListBranches(getClient GetClientFn, t translations.TranslationHelperFunc) (
310310
// CreateOrUpdateFile creates a tool to create or update a file in a GitHub repository.
311311
func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
312312
tool := mcp.Tool{
313-
Name: "create_or_update_file",
314-
Description: 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. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations."),
313+
Name: "create_or_update_file",
314+
Description: t("TOOL_CREATE_OR_UPDATE_FILE_DESCRIPTION", `Create or update a single file in a GitHub repository.
315+
If updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.
316+
317+
In order to obtain the SHA of original file version before updating, use the following git command:
318+
git ls-tree HEAD <path to file>
319+
320+
If the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval.
321+
`),
315322
Annotations: &mcp.ToolAnnotations{
316323
Title: t("TOOL_CREATE_OR_UPDATE_FILE_USER_TITLE", "Create or update file"),
317324
ReadOnlyHint: false,
@@ -345,7 +352,7 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
345352
},
346353
"sha": {
347354
Type: "string",
348-
Description: "Required if updating an existing file. The blob SHA of the file being replaced.",
355+
Description: "The blob SHA of the file being replaced.",
349356
},
350357
},
351358
Required: []string{"owner", "repo", "path", "content", "message", "branch"},
@@ -404,6 +411,58 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
404411
}
405412

406413
path = strings.TrimPrefix(path, "/")
414+
415+
// SHA validation using conditional HEAD request (efficient - no body transfer)
416+
var previousSHA string
417+
contentURL := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(path))
418+
if branch != "" {
419+
contentURL += "?ref=" + url.QueryEscape(branch)
420+
}
421+
422+
if sha != "" {
423+
// User provided SHA - validate it's still current
424+
req, err := client.NewRequest("HEAD", contentURL, nil)
425+
if err == nil {
426+
req.Header.Set("If-None-Match", fmt.Sprintf(`"%s"`, sha))
427+
resp, _ := client.Do(ctx, req, nil)
428+
if resp != nil {
429+
defer resp.Body.Close()
430+
431+
switch resp.StatusCode {
432+
case http.StatusNotModified:
433+
// SHA matches current - proceed
434+
opts.SHA = github.Ptr(sha)
435+
case http.StatusOK:
436+
// SHA is stale - reject with current SHA so user can check diff
437+
currentSHA := strings.Trim(resp.Header.Get("ETag"), `"`)
438+
return utils.NewToolResultError(fmt.Sprintf(
439+
"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
440+
"Use get_file_contents or compare commits to review changes before updating.",
441+
sha, currentSHA)), nil, nil
442+
case http.StatusNotFound:
443+
// File doesn't exist - this is a create, ignore provided SHA
444+
}
445+
}
446+
}
447+
} else {
448+
// No SHA provided - check if file exists to warn about blind update
449+
req, err := client.NewRequest("HEAD", contentURL, nil)
450+
if err == nil {
451+
resp, _ := client.Do(ctx, req, nil)
452+
if resp != nil {
453+
defer resp.Body.Close()
454+
if resp.StatusCode == http.StatusOK {
455+
previousSHA = strings.Trim(resp.Header.Get("ETag"), `"`)
456+
}
457+
// 404 = new file, no previous SHA needed
458+
}
459+
}
460+
}
461+
462+
if previousSHA != "" {
463+
opts.SHA = github.Ptr(previousSHA)
464+
}
465+
407466
fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts)
408467
if err != nil {
409468
return ghErrors.NewGitHubAPIErrorResponse(ctx,
@@ -427,6 +486,19 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
427486
return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
428487
}
429488

489+
// Warn if file was updated without SHA validation (blind update)
490+
if sha == "" && previousSHA != "" {
491+
return utils.NewToolResultText(fmt.Sprintf(
492+
"Warning: File updated without SHA validation. Previous file SHA was %s. "+
493+
`Verify no unintended changes were overwritten:
494+
1. Extract the SHA of the local version using git ls-tree HEAD %s.
495+
2. Compare with the previous SHA above.
496+
3. Revert changes if shas do not match.
497+
498+
%s`,
499+
previousSHA, path, string(r))), nil, nil
500+
}
501+
430502
return utils.NewToolResultText(string(r)), nil, nil
431503
})
432504

‎pkg/github/repositories_test.go‎

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,6 +1121,182 @@ func Test_CreateOrUpdateFile(t *testing.T) {
11211121
expectError: true,
11221122
expectedErrMsg: "failed to create/update file",
11231123
},
1124+
{
1125+
name: "sha validation - current sha matches (304 Not Modified)",
1126+
mockedClient: mock.NewMockedHTTPClient(
1127+
mock.WithRequestMatchHandler(
1128+
mock.EndpointPattern{
1129+
Pattern: "/repos/owner/repo/contents/docs/example.md",
1130+
Method: "HEAD",
1131+
},
1132+
http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
1133+
// Verify If-None-Match header is set correctly
1134+
ifNoneMatch := req.Header.Get("If-None-Match")
1135+
if ifNoneMatch == `"abc123def456"` {
1136+
w.WriteHeader(http.StatusNotModified)
1137+
} else {
1138+
w.WriteHeader(http.StatusOK)
1139+
w.Header().Set("ETag", `"abc123def456"`)
1140+
}
1141+
}),
1142+
),
1143+
mock.WithRequestMatchHandler(
1144+
mock.PutReposContentsByOwnerByRepoByPath,
1145+
expectRequestBody(t, map[string]interface{}{
1146+
"message": "Update example file",
1147+
"content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==",
1148+
"branch": "main",
1149+
"sha": "abc123def456",
1150+
}).andThen(
1151+
mockResponse(t, http.StatusOK, mockFileResponse),
1152+
),
1153+
),
1154+
),
1155+
requestArgs: map[string]interface{}{
1156+
"owner": "owner",
1157+
"repo": "repo",
1158+
"path": "docs/example.md",
1159+
"content": "# Updated Example\n\nThis file has been updated.",
1160+
"message": "Update example file",
1161+
"branch": "main",
1162+
"sha": "abc123def456",
1163+
},
1164+
expectError: false,
1165+
expectedContent: mockFileResponse,
1166+
},
1167+
{
1168+
name: "sha validation - stale sha detected (200 OK with different ETag)",
1169+
mockedClient: mock.NewMockedHTTPClient(
1170+
mock.WithRequestMatchHandler(
1171+
mock.EndpointPattern{
1172+
Pattern: "/repos/owner/repo/contents/docs/example.md",
1173+
Method: "HEAD",
1174+
},
1175+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1176+
// SHA doesn't match - return 200 with current ETag
1177+
w.Header().Set("ETag", `"newsha999888"`)
1178+
w.WriteHeader(http.StatusOK)
1179+
}),
1180+
),
1181+
),
1182+
requestArgs: map[string]interface{}{
1183+
"owner": "owner",
1184+
"repo": "repo",
1185+
"path": "docs/example.md",
1186+
"content": "# Updated Example\n\nThis file has been updated.",
1187+
"message": "Update example file",
1188+
"branch": "main",
1189+
"sha": "oldsha123456",
1190+
},
1191+
expectError: true,
1192+
expectedErrMsg: "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888",
1193+
},
1194+
{
1195+
name: "sha validation - file doesn't exist (404), proceed with create",
1196+
mockedClient: mock.NewMockedHTTPClient(
1197+
mock.WithRequestMatchHandler(
1198+
mock.EndpointPattern{
1199+
Pattern: "/repos/owner/repo/contents/docs/example.md",
1200+
Method: "HEAD",
1201+
},
1202+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1203+
w.WriteHeader(http.StatusNotFound)
1204+
}),
1205+
),
1206+
mock.WithRequestMatchHandler(
1207+
mock.PutReposContentsByOwnerByRepoByPath,
1208+
expectRequestBody(t, map[string]interface{}{
1209+
"message": "Create new file",
1210+
"content": "IyBOZXcgRmlsZQoKVGhpcyBpcyBhIG5ldyBmaWxlLg==",
1211+
"branch": "main",
1212+
"sha": "ignoredsha", // SHA is sent but GitHub API ignores it for new files
1213+
}).andThen(
1214+
mockResponse(t, http.StatusCreated, mockFileResponse),
1215+
),
1216+
),
1217+
),
1218+
requestArgs: map[string]interface{}{
1219+
"owner": "owner",
1220+
"repo": "repo",
1221+
"path": "docs/example.md",
1222+
"content": "# New File\n\nThis is a new file.",
1223+
"message": "Create new file",
1224+
"branch": "main",
1225+
"sha": "ignoredsha",
1226+
},
1227+
expectError: false,
1228+
expectedContent: mockFileResponse,
1229+
},
1230+
{
1231+
name: "no sha provided - file exists, returns warning",
1232+
mockedClient: mock.NewMockedHTTPClient(
1233+
mock.WithRequestMatchHandler(
1234+
mock.EndpointPattern{
1235+
Pattern: "/repos/owner/repo/contents/docs/example.md",
1236+
Method: "HEAD",
1237+
},
1238+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1239+
w.Header().Set("ETag", `"existing123"`)
1240+
w.WriteHeader(http.StatusOK)
1241+
}),
1242+
),
1243+
mock.WithRequestMatchHandler(
1244+
mock.PutReposContentsByOwnerByRepoByPath,
1245+
expectRequestBody(t, map[string]interface{}{
1246+
"message": "Update without SHA",
1247+
"content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==",
1248+
"branch": "main",
1249+
"sha": "existing123", // SHA is automatically added from ETag
1250+
}).andThen(
1251+
mockResponse(t, http.StatusOK, mockFileResponse),
1252+
),
1253+
),
1254+
),
1255+
requestArgs: map[string]interface{}{
1256+
"owner": "owner",
1257+
"repo": "repo",
1258+
"path": "docs/example.md",
1259+
"content": "# Updated\n\nUpdated without SHA.",
1260+
"message": "Update without SHA",
1261+
"branch": "main",
1262+
},
1263+
expectError: false,
1264+
expectedErrMsg: "Warning: File updated without SHA validation. Previous file SHA was existing123",
1265+
},
1266+
{
1267+
name: "no sha provided - file doesn't exist, no warning",
1268+
mockedClient: mock.NewMockedHTTPClient(
1269+
mock.WithRequestMatchHandler(
1270+
mock.EndpointPattern{
1271+
Pattern: "/repos/owner/repo/contents/docs/example.md",
1272+
Method: "HEAD",
1273+
},
1274+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1275+
w.WriteHeader(http.StatusNotFound)
1276+
}),
1277+
),
1278+
mock.WithRequestMatchHandler(
1279+
mock.PutReposContentsByOwnerByRepoByPath,
1280+
expectRequestBody(t, map[string]interface{}{
1281+
"message": "Create new file",
1282+
"content": "IyBOZXcgRmlsZQoKQ3JlYXRlZCB3aXRob3V0IFNIQQ==",
1283+
"branch": "main",
1284+
}).andThen(
1285+
mockResponse(t, http.StatusCreated, mockFileResponse),
1286+
),
1287+
),
1288+
),
1289+
requestArgs: map[string]interface{}{
1290+
"owner": "owner",
1291+
"repo": "repo",
1292+
"path": "docs/example.md",
1293+
"content": "# New File\n\nCreated without SHA",
1294+
"message": "Create new file",
1295+
"branch": "main",
1296+
},
1297+
expectError: false,
1298+
expectedContent: mockFileResponse,
1299+
},
11241300
}
11251301

11261302
for _, tc := range tests {
@@ -1150,6 +1326,12 @@ func Test_CreateOrUpdateFile(t *testing.T) {
11501326
// Parse the result and get the text content if no error
11511327
textContent := getTextResult(t, result)
11521328

1329+
// If expectedErrMsg is set (but expectError is false), this is a warning case
1330+
if tc.expectedErrMsg != "" {
1331+
assert.Contains(t, textContent.Text, tc.expectedErrMsg)
1332+
return
1333+
}
1334+
11531335
// Unmarshal and verify the result
11541336
var returnedContent github.RepositoryContentResponse
11551337
err = json.Unmarshal([]byte(textContent.Text), &returnedContent)

0 commit comments

Comments
 (0)