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

README.md

@@ -1272,7 +1272,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`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional)
+  - `sha`: The blob SHA of the file being replaced. Required if the file already exists. Retrieve it with get_file_contents using the same owner, repo, and path, with ref set to this tool's branch value. (string, optional)
 
 - **create_repository** - Create repository
   - **OAuth Challenge Scopes**: `repo`

docs/feature-flags.md

@@ -357,7 +357,7 @@ runtime behavior (such as output formatting) won't appear here.
 ### `thread_resolution_reason`
 
 - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews
-  - **Required OAuth Scopes**: `repo`
+  - **OAuth Challenge Scopes**: `repo`
   - `body`: Review comment text (string, optional)
   - `commitID`: SHA of commit to review (string, optional)
   - `event`: Review action to perform. (string, optional)

pkg/github/__toolsnaps__/create_or_update_file.snap

@@ -4,7 +4,7 @@
     "readOnlyHint": false,
     "title": "Create or update file"
   },
-  "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 rev-parse \u003cbranch\u003e:\u003cpath to file\u003e\n\nSHA MUST be provided for existing file updates.\n",
+  "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\nTo obtain the current blob SHA before updating, call the get_file_contents tool with the same owner, repo, and path, and set its ref parameter to this tool's branch value. The first text result reports the blob SHA for the requested path.\n\nSHA MUST be provided for existing file updates.\n",
   "inputSchema": {
     "properties": {
       "allow_symlink_write": {
@@ -37,7 +37,7 @@
         "type": "string"
       },
       "sha": {
-        "description": "The blob SHA of the file being replaced. Required if the file already exists.",
+        "description": "The blob SHA of the file being replaced. Required if the file already exists. Retrieve it with get_file_contents using the same owner, repo, and path, with ref set to this tool's branch value.",
         "type": "string"
       }
     },

pkg/github/repositories.go

@@ -412,8 +412,7 @@ func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTo
 			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 rev-parse <branch>:<path to file>
+To obtain the current blob SHA before updating, call the get_file_contents tool with the same owner, repo, and path, and set its ref parameter to this tool's branch value. The first text result reports the blob SHA for the requested path.
 
 SHA MUST be provided for existing file updates.
 `),
@@ -450,7 +449,7 @@ SHA MUST be provided for existing file updates.
 					},
 					"sha": {
 						Type:        "string",
-						Description: "The blob SHA of the file being replaced. Required if the file already exists.",
+						Description: "The blob SHA of the file being replaced. Required if the file already exists. Retrieve it with get_file_contents using the same owner, repo, and path, with ref set to this tool's branch value.",
 					},
 					"allow_symlink_write": {
 						Type:        "boolean",
@@ -551,8 +550,10 @@ 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. "+
-								"Pull the latest changes and use git rev-parse %s:%s to get the current SHA.",
-							sha, currentSHA, branch, path)), nil, nil
+								"The file changed since you read it. Call get_file_contents with owner=%q, repo=%q, path=%q, and ref=%q; "+
+								"its first text result reports the blob SHA for the requested path. "+
+								"Rebuild your content against what it returns, and retry with the sha parameter set to the SHA that call reports.",
+							sha, currentSHA, owner, repo, path, branch)), nil, nil
 					}
 					if !allowSymlinkWrite {
 						if existingFile.GetType() == "symlink" {
@@ -596,8 +597,10 @@ 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. "+
-							"Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.",
-						path, branch, path)), nil, nil
+							"Call get_file_contents with owner=%q, repo=%q, path=%q, and ref=%q to read the file you are about to overwrite; "+
+							"its first text result reports the blob SHA for the requested path. "+
+							"Then retry with the sha parameter set to the blob SHA that call reports.",
+						path, owner, repo, path, branch)), nil, nil
 				}
 				// If file not found, no previous SHA needed (new file creation)
 			}

pkg/github/repositories_test.go

@@ -170,6 +170,7 @@ func Test_GetFileContents(t *testing.T) {
 				Text:     "# Test Repository\n\nThis is a test repository.",
 				MIMEType: "text/plain; charset=utf-8",
 			},
+			expectedMsg: "SHA: " + gitBlobSHA(mockRawContent),
 		},
 		{
 			name: "successful binary file content fetch (PNG)",
@@ -625,6 +626,7 @@ func Test_GetFileContents_SymlinkDisclosure(t *testing.T) {
 			metadata := repositoryPathMetadataFromResult(t, result)
 			assert.Equal(t, "symlink", metadata.Type)
 			assert.Equal(t, "docs/link", metadata.Path)
+			assert.Equal(t, linkSHA, metadata.SHA)
 			assert.Equal(t, target, metadata.Target)
 			assert.Equal(t, "target/"+tc.name, metadata.ResolvedTargetPath)
 			assert.Equal(t, "dereferenced_target", metadata.Content)
@@ -649,6 +651,7 @@ func Test_GetFileContents_SymlinkDisclosure(t *testing.T) {
 		require.False(t, result.IsError)
 		assert.Equal(t, 1, requests)
 		metadata := repositoryPathMetadataFromResult(t, result)
+		assert.Equal(t, gitBlobSHA([]byte(target)), metadata.SHA)
 		assert.Equal(t, target, metadata.Target)
 		assert.Empty(t, metadata.ResolvedTargetPath)
 		assert.Equal(t, "not_returned", metadata.Content)
@@ -2057,6 +2060,9 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 
 	assert.Equal(t, "create_or_update_file", tool.Name)
 	assert.NotEmpty(t, tool.Description)
+	assert.NotContains(t, tool.Description, "git rev-parse")
+	assert.Contains(t, tool.Description, "get_file_contents")
+	assert.Contains(t, tool.Description, "set its ref parameter to this tool's branch value")
 	assert.Contains(t, schema.Properties, "owner")
 	assert.Contains(t, schema.Properties, "repo")
 	assert.Contains(t, schema.Properties, "path")
@@ -2065,6 +2071,7 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 	assert.Contains(t, schema.Properties, "branch")
 	assert.Contains(t, schema.Properties, "sha")
 	assert.Contains(t, schema.Properties, "allow_symlink_write")
+	assert.Contains(t, schema.Properties["sha"].Description, "with ref set to this tool's branch value")
 	assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "content", "message", "branch"})
 
 	// Setup mock file content response
@@ -2135,6 +2142,7 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 		expectedContent      *github.RepositoryContentResponse
 		expectedErrMsg       string
 		expectedErrMsgs      []string
+		unexpectedErrMsgs    []string
 		expectedRequestCount int
 	}{
 		{
@@ -2449,26 +2457,60 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 		{
 			name: "sha validation - stale sha detected",
 			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
-				"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
-					SHA:  github.Ptr("newsha999888"),
-					Type: github.Ptr("file"),
+				"GET /repos/owner/repo/contents/docs/example.md": expectQueryParams(t, map[string]string{
+					"ref": "main",
+				}).andThen(mockResponse(t, http.StatusOK, &github.RepositoryContent{
+					SHA:    github.Ptr(symlinkSHA),
+					Type:   github.Ptr("symlink"),
+					Target: github.Ptr(string(symlinkTarget)),
+				})),
+				"GET /repos/{owner}/{repo}/contents/{path:.*}": expectQueryParams(t, map[string]string{
+					"ref": "main",
+				}).andThen(mockResponse(t, http.StatusOK, &github.RepositoryContent{
+					SHA:    github.Ptr(symlinkSHA),
+					Type:   github.Ptr("symlink"),
+					Target: github.Ptr(string(symlinkTarget)),
+				})),
+			}),
+			requestArgs: map[string]any{
+				"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,
+			expectedErrMsgs: []string{
+				"SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is " + symlinkSHA,
+				`Call get_file_contents with owner="owner", repo="repo", path="docs/example.md", and ref="main"`,
+				"its first text result reports the blob SHA for the requested path",
+				"retry with the sha parameter set to the SHA that call reports",
+			},
+			expectedRequestCount: 1,
+		},
+		{
+			name: "sha validation - api error is surfaced",
+			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+				"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusForbidden, map[string]any{
+					"message": "Resource not accessible",
 				}),
-				"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
-					SHA:  github.Ptr("newsha999888"),
-					Type: github.Ptr("file"),
+				"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusForbidden, map[string]any{
+					"message": "Resource not accessible",
 				}),
 			}),
 			requestArgs: map[string]any{
 				"owner":   "owner",
 				"repo":    "repo",
 				"path":    "docs/example.md",
-				"content": "# Updated Example\n\nThis file has been updated.",
+				"content": "updated",
 				"message": "Update example file",
 				"branch":  "main",
 				"sha":     "oldsha123456",
 			},
 			expectError:          true,
-			expectedErrMsg:       "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888",
+			expectedErrMsg:       "failed to verify file SHA",
 			expectedRequestCount: 1,
 		},
 		{
@@ -2513,25 +2555,59 @@ func Test_CreateOrUpdateFile(t *testing.T) {
 		{
 			name: "no sha provided - file exists, rejects update",
 			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
-				"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
+				"GET /repos/owner/repo/contents/docs/example.md": expectQueryParams(t, map[string]string{
+					"ref": "release/#candidate",
+				}).andThen(mockResponse(t, http.StatusOK, &github.RepositoryContent{
 					SHA:  github.Ptr("existing123"),
 					Type: github.Ptr("file"),
-				}),
-				"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
+				})),
+				"GET /repos/{owner}/{repo}/contents/{path:.*}": expectQueryParams(t, map[string]string{
+					"ref": "release/#candidate",
+				}).andThen(mockResponse(t, http.StatusOK, &github.RepositoryContent{
 					SHA:  github.Ptr("existing123"),
 					Type: github.Ptr("file"),
-				}),
+				})),
 			}),
 			requestArgs: map[string]any{
 				"owner":   "owner",
 				"repo":    "repo",
 				"path":    "docs/example.md",
 				"content": "# Updated\n\nUpdated without SHA.",
 				"message": "Update without SHA",
+				"branch":  "release/#candidate",
+			},
+			expectError: true,
+			expectedErrMsgs: []string{
+				"File already exists at docs/example.md",
+				`Call get_file_contents with owner="owner", repo="repo", path="docs/example.md", and ref="release/#candidate"`,
+				"its first text result reports the blob SHA for the requested path",
+				"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,
+		},
+		{
+			name: "no sha provided - api error is surfaced",
+			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+				"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusInternalServerError, map[string]any{
+					"message": "Internal Server Error",
+				}),
+				"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusInternalServerError, map[string]any{
+					"message": "Internal Server Error",
+				}),
+			}),
+			requestArgs: map[string]any{
+				"owner":   "owner",
+				"repo":    "repo",
+				"path":    "docs/example.md",
+				"content": "updated",
+				"message": "Update example file",
 				"branch":  "main",
 			},
 			expectError:          true,
-			expectedErrMsg:       "File already exists at docs/example.md",
+			expectedErrMsg:       "failed to check if file exists",
 			expectedRequestCount: 1,
 		},
 		{
@@ -2606,6 +2682,12 @@ 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")
 				return
 			}