Fix create_or_update SHA-related failures (#1621)

almaleksia · web-flow · commit afe34d820043 · 2025-12-16T15:44:52.000+01:00
* Add repos toolset instructions

* 1. Make sha optional (if not supplied - GetContents is used to retrieve original sha)
2. Instruct LLM to supply actual sha using git command and move instructions to tool description.

* Update toolsnaps and docs

* Better error handling

* Add tests

* Clearing resources in time

* Addressing review comments &amp;&amp; checking etag

* Test fixes
diff --git a/README.md b/README.md
@@ -1061,7 +1061,7 @@ The following sets of tools are available:
   - `owner`: Repository owner (username or organization) (string, required)
   - `path`: Path where to create/update the file (string, required)
   - `repo`: Repository name (string, required)
-  - `sha`: Required if updating an existing file. The blob SHA of the file being replaced. (string, optional)
+  - `sha`: The blob SHA of the file being replaced. (string, optional)
 
 - **create_repository** - Create repository
   - `autoInit`: Initialize with README (boolean, optional)
diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap
@@ -2,7 +2,7 @@
   "annotations": {
     "title": "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.",
+  "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",
   "inputSchema": {
     "type": "object",
     "required": [
@@ -40,7 +40,7 @@
       },
       "sha": {
         "type": "string",
-        "description": "Required if updating an existing file. The blob SHA of the file being replaced."
+        "description": "The blob SHA of the file being replaced."
       }
     }
   },
diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go
@@ -310,8 +310,15 @@ 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) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
 	tool := mcp.Tool{
-		Name:        "create_or_update_file",
-		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."),
+		Name: "create_or_update_file",
+		Description: t("TOOL_CREATE_OR_UPDATE_FILE_DESCRIPTION", `Create or update a single file in a GitHub repository. 
+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.
+
+In order to obtain the SHA of original file version before updating, use the following git command:
+git ls-tree HEAD <path to file>
+
+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.
+`),
 		Annotations: &mcp.ToolAnnotations{
 			Title:        t("TOOL_CREATE_OR_UPDATE_FILE_USER_TITLE", "Create or update file"),
 			ReadOnlyHint: false,
@@ -345,7 +352,7 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
 				},
 				"sha": {
 					Type:        "string",
-					Description: "Required if updating an existing file. The blob SHA of the file being replaced.",
+					Description: "The blob SHA of the file being replaced.",
 				},
 			},
 			Required: []string{"owner", "repo", "path", "content", "message", "branch"},
@@ -404,6 +411,58 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
 		}
 
 		path = strings.TrimPrefix(path, "/")
+
+		// SHA validation using conditional HEAD request (efficient - no body transfer)
+		var previousSHA string
+		contentURL := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(path))
+		if branch != "" {
+			contentURL += "?ref=" + url.QueryEscape(branch)
+		}
+
+		if sha != "" {
+			// User provided SHA - validate it's still current
+			req, err := client.NewRequest("HEAD", contentURL, nil)
+			if err == nil {
+				req.Header.Set("If-None-Match", fmt.Sprintf(`"%s"`, sha))
+				resp, _ := client.Do(ctx, req, nil)
+				if resp != nil {
+					defer resp.Body.Close()
+
+					switch resp.StatusCode {
+					case http.StatusNotModified:
+						// SHA matches current - proceed
+						opts.SHA = github.Ptr(sha)
+					case http.StatusOK:
+						// SHA is stale - reject with current SHA so user can check diff
+						currentSHA := strings.Trim(resp.Header.Get("ETag"), `"`)
+						return utils.NewToolResultError(fmt.Sprintf(
+							"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
+								"Use get_file_contents or compare commits to review changes before updating.",
+							sha, currentSHA)), nil, nil
+					case http.StatusNotFound:
+						// File doesn't exist - this is a create, ignore provided SHA
+					}
+				}
+			}
+		} else {
+			// No SHA provided - check if file exists to warn about blind update
+			req, err := client.NewRequest("HEAD", contentURL, nil)
+			if err == nil {
+				resp, _ := client.Do(ctx, req, nil)
+				if resp != nil {
+					defer resp.Body.Close()
+					if resp.StatusCode == http.StatusOK {
+						previousSHA = strings.Trim(resp.Header.Get("ETag"), `"`)
+					}
+					// 404 = new file, no previous SHA needed
+				}
+			}
+		}
+
+		if previousSHA != "" {
+			opts.SHA = github.Ptr(previousSHA)
+		}
+
 		fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts)
 		if err != nil {
 			return ghErrors.NewGitHubAPIErrorResponse(ctx,
@@ -427,6 +486,19 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF
 			return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
 		}
 
+		// Warn if file was updated without SHA validation (blind update)
+		if sha == "" && previousSHA != "" {
+			return utils.NewToolResultText(fmt.Sprintf(
+				"Warning: File updated without SHA validation. Previous file SHA was %s. "+
+					`Verify no unintended changes were overwritten: 
+1. Extract the SHA of the local version using git ls-tree HEAD %s.
+2. Compare with the previous SHA above.
+3. Revert changes if shas do not match.
+
+%s`,
+				previousSHA, path, string(r))), nil, nil
+		}
+
 		return utils.NewToolResultText(string(r)), nil, nil
 	})
 
diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go
@@ -1121,6 +1121,182 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 			expectError:    true,
 			expectedErrMsg: "failed to create/update file",
 		},
+		{
+			name: "sha validation - current sha matches (304 Not Modified)",
+			mockedClient: mock.NewMockedHTTPClient(
+				mock.WithRequestMatchHandler(
+					mock.EndpointPattern{
+						Pattern: "/repos/owner/repo/contents/docs/example.md",
+						Method:  "HEAD",
+					},
+					http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+						// Verify If-None-Match header is set correctly
+						ifNoneMatch := req.Header.Get("If-None-Match")
+						if ifNoneMatch == `"abc123def456"` {
+							w.WriteHeader(http.StatusNotModified)
+						} else {
+							w.WriteHeader(http.StatusOK)
+							w.Header().Set("ETag", `"abc123def456"`)
+						}
+					}),
+				),
+				mock.WithRequestMatchHandler(
+					mock.PutReposContentsByOwnerByRepoByPath,
+					expectRequestBody(t, map[string]interface{}{
+						"message": "Update example file",
+						"content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==",
+						"branch":  "main",
+						"sha":     "abc123def456",
+					}).andThen(
+						mockResponse(t, http.StatusOK, mockFileResponse),
+					),
+				),
+			),
+			requestArgs: map[string]interface{}{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "# Updated Example\n\nThis file has been updated.",
+				"message": "Update example file",
+				"branch":  "main",
+				"sha":     "abc123def456",
+			},
+			expectError:     false,
+			expectedContent: mockFileResponse,
+		},
+		{
+			name: "sha validation - stale sha detected (200 OK with different ETag)",
+			mockedClient: mock.NewMockedHTTPClient(
+				mock.WithRequestMatchHandler(
+					mock.EndpointPattern{
+						Pattern: "/repos/owner/repo/contents/docs/example.md",
+						Method:  "HEAD",
+					},
+					http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+						// SHA doesn't match - return 200 with current ETag
+						w.Header().Set("ETag", `"newsha999888"`)
+						w.WriteHeader(http.StatusOK)
+					}),
+				),
+			),
+			requestArgs: map[string]interface{}{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "# Updated Example\n\nThis file has been updated.",
+				"message": "Update example file",
+				"branch":  "main",
+				"sha":     "oldsha123456",
+			},
+			expectError:    true,
+			expectedErrMsg: "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888",
+		},
+		{
+			name: "sha validation - file doesn't exist (404), proceed with create",
+			mockedClient: mock.NewMockedHTTPClient(
+				mock.WithRequestMatchHandler(
+					mock.EndpointPattern{
+						Pattern: "/repos/owner/repo/contents/docs/example.md",
+						Method:  "HEAD",
+					},
+					http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+						w.WriteHeader(http.StatusNotFound)
+					}),
+				),
+				mock.WithRequestMatchHandler(
+					mock.PutReposContentsByOwnerByRepoByPath,
+					expectRequestBody(t, map[string]interface{}{
+						"message": "Create new file",
+						"content": "IyBOZXcgRmlsZQoKVGhpcyBpcyBhIG5ldyBmaWxlLg==",
+						"branch":  "main",
+						"sha":     "ignoredsha", // SHA is sent but GitHub API ignores it for new files
+					}).andThen(
+						mockResponse(t, http.StatusCreated, mockFileResponse),
+					),
+				),
+			),
+			requestArgs: map[string]interface{}{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "# New File\n\nThis is a new file.",
+				"message": "Create new file",
+				"branch":  "main",
+				"sha":     "ignoredsha",
+			},
+			expectError:     false,
+			expectedContent: mockFileResponse,
+		},
+		{
+			name: "no sha provided - file exists, returns warning",
+			mockedClient: mock.NewMockedHTTPClient(
+				mock.WithRequestMatchHandler(
+					mock.EndpointPattern{
+						Pattern: "/repos/owner/repo/contents/docs/example.md",
+						Method:  "HEAD",
+					},
+					http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+						w.Header().Set("ETag", `"existing123"`)
+						w.WriteHeader(http.StatusOK)
+					}),
+				),
+				mock.WithRequestMatchHandler(
+					mock.PutReposContentsByOwnerByRepoByPath,
+					expectRequestBody(t, map[string]interface{}{
+						"message": "Update without SHA",
+						"content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==",
+						"branch":  "main",
+						"sha":     "existing123", // SHA is automatically added from ETag
+					}).andThen(
+						mockResponse(t, http.StatusOK, mockFileResponse),
+					),
+				),
+			),
+			requestArgs: map[string]interface{}{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "# Updated\n\nUpdated without SHA.",
+				"message": "Update without SHA",
+				"branch":  "main",
+			},
+			expectError:    false,
+			expectedErrMsg: "Warning: File updated without SHA validation. Previous file SHA was existing123",
+		},
+		{
+			name: "no sha provided - file doesn't exist, no warning",
+			mockedClient: mock.NewMockedHTTPClient(
+				mock.WithRequestMatchHandler(
+					mock.EndpointPattern{
+						Pattern: "/repos/owner/repo/contents/docs/example.md",
+						Method:  "HEAD",
+					},
+					http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+						w.WriteHeader(http.StatusNotFound)
+					}),
+				),
+				mock.WithRequestMatchHandler(
+					mock.PutReposContentsByOwnerByRepoByPath,
+					expectRequestBody(t, map[string]interface{}{
+						"message": "Create new file",
+						"content": "IyBOZXcgRmlsZQoKQ3JlYXRlZCB3aXRob3V0IFNIQQ==",
+						"branch":  "main",
+					}).andThen(
+						mockResponse(t, http.StatusCreated, mockFileResponse),
+					),
+				),
+			),
+			requestArgs: map[string]interface{}{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "# New File\n\nCreated without SHA",
+				"message": "Create new file",
+				"branch":  "main",
+			},
+			expectError:     false,
+			expectedContent: mockFileResponse,
+		},
 	}
 
 	for _, tc := range tests {
@@ -1150,6 +1326,12 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 			// Parse the result and get the text content if no error
 			textContent := getTextResult(t, result)
 
+			// If expectedErrMsg is set (but expectError is false), this is a warning case
+			if tc.expectedErrMsg != "" {
+				assert.Contains(t, textContent.Text, tc.expectedErrMsg)
+				return
+			}
+
 			// Unmarshal and verify the result
 			var returnedContent github.RepositoryContentResponse
 			err = json.Unmarshal([]byte(textContent.Text), &returnedContent)
