diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 37bf3b46bf..272bc12438 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/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 SHA of the current file version before updating, call the get_file_contents tool for the same path and ref; it reports the blob SHA of the file it returns.\n\nSHA MUST be provided for existing file updates.\n", "inputSchema": { "properties": { "allow_symlink_write": { diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 446d7ca551..a2da0df91d 100644 --- a/pkg/github/repositories.go +++ b/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 : +To obtain the SHA of the current file version before updating, call the get_file_contents tool for the same path and ref; it reports the blob SHA of the file it returns. SHA MUST be provided for existing file updates. `), @@ -549,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. "+ - "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, 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" { @@ -594,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. "+ - "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 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) } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index e45c3c4f47..72f7a82188 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2058,6 +2058,8 @@ 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, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "path") @@ -2136,6 +2138,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent *github.RepositoryContentResponse expectedErrMsg string expectedErrMsgs []string + unexpectedErrMsgs []string expectedRequestCount int }{ { @@ -2468,8 +2471,12 @@ func Test_CreateOrUpdateFile(t *testing.T) { "branch": "main", "sha": "oldsha123456", }, - expectError: true, - expectedErrMsg: "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888", + expectError: true, + expectedErrMsgs: []string{ + "SHA mismatch: provided SHA oldsha123456 is stale. Current file SHA is newsha999888", + "re-read it with get_file_contents", + "retry with the sha parameter set to the SHA that call reports", + }, expectedRequestCount: 1, }, { @@ -2531,8 +2538,15 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Update without SHA", "branch": "main", }, - expectError: true, - expectedErrMsg: "File already exists at docs/example.md", + expectError: true, + expectedErrMsgs: []string{ + "File already exists at docs/example.md", + "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, }, { @@ -2607,6 +2621,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 }