diff --git a/src/kimi_cli/tools/file/replace.py b/src/kimi_cli/tools/file/replace.py index 4f551de4f..122a95146 100644 --- a/src/kimi_cli/tools/file/replace.py +++ b/src/kimi_cli/tools/file/replace.py @@ -134,15 +134,61 @@ async def __call__(self, params: Params) -> ToolReturnValue: original_content = content edits = [params.edit] if isinstance(params.edit, Edit) else params.edit - # Apply all edits - for edit in edits: + # Apply edits sequentially. Each edit applies to the result of the + # previous one, so a later edit may legitimately target text an + # earlier edit introduced. Validate that every edit's old string is + # present at its turn and fail the whole call otherwise: silently + # skipping a non-matching edit while still reporting success drops + # requested changes without the model noticing. + total_replacements = 0 + for index, edit in enumerate(edits): + # Reject an empty old string: `str.count('')` returns + # `len(content) + 1` (never 0), so the occurrence check below + # cannot catch it, and `str.replace('', new)` would splice + # `new` between every character. + if edit.old == "": + detail = ( + "old string must not be empty." + if len(edits) == 1 + else f"Edit #{index + 1}'s old string must not be empty." + ) + return ToolError( + message=f"No replacements were made. {detail}", + brief="Empty old string", + ) + occurrences = content.count(edit.old) + if occurrences == 0: + detail = ( + "The old string was not found in the file." + if len(edits) == 1 + else ( + f"Edit #{index + 1}'s old string was not found " + "(an earlier edit in this call may have changed that text)." + ) + ) + return ToolError( + message=( + f"No replacements were made. {detail} " + "The file contents may be out of date; " + "use the Read tool to reload it." + ), + brief="No replacements made", + ) content = self._apply_edit(content, edit) + total_replacements += occurrences if edit.replace_all else 1 - # Check if any changes were made + # Every edit matched (validated above) but the result is identical + # to the original — e.g. an edit whose old string equals its new + # string, or edits that cancel out. Report a no-op rather than the + # misleading "old string was not found", which would send the model + # re-reading the file for a string that is actually present. if content == original_content: return ToolError( - message="No replacements were made. The old string was not found in the file.", - brief="No replacements made", + message=( + "No replacements were made: the edit(s) matched but left the file " + "unchanged (an old string equals its new string, or the edits cancel out)." + ), + brief="No changes made", ) diff_blocks: list[DisplayBlock] = await build_diff_blocks( @@ -169,14 +215,6 @@ async def __call__(self, params: Params) -> ToolReturnValue: # Write the modified content back to the file await p.write_text(content, errors="replace") - # Count changes for success message - total_replacements = 0 - for edit in edits: - if edit.replace_all: - total_replacements += original_content.count(edit.old) - else: - total_replacements += 1 if edit.old in original_content else 0 - return ToolReturnValue( is_error=False, output="", diff --git a/tests/tools/test_str_replace_file.py b/tests/tools/test_str_replace_file.py index a16dad303..4cfb2bef3 100644 --- a/tests/tools/test_str_replace_file.py +++ b/tests/tools/test_str_replace_file.py @@ -246,3 +246,90 @@ async def test_replace_empty_strings( assert not result.is_error assert "successfully edited" in result.message assert await file_path.read_text() == "Hello !" + + +async def test_replace_multiple_edits_one_not_found_is_atomic( + str_replace_file_tool: StrReplaceFile, temp_work_dir: KaosPath +): + """A multi-edit where one edit's old string is missing must fail the whole + call, not silently apply the matching edits and report success.""" + file_path = temp_work_dir / "test.txt" + original_content = "Hello world! Goodbye world!" + await file_path.write_text(original_content) + + result = await str_replace_file_tool( + Params( + path=str(file_path), + edit=[ + Edit(old="Hello", new="Hi"), + Edit(old="does-not-exist", new="x"), + ], + ) + ) + + assert result.is_error + assert "No replacements were made" in result.message + # The whole batch fails atomically — the file is left untouched even though + # the first edit on its own would have matched. + assert await file_path.read_text() == original_content + + +async def test_replace_sequential_dependent_edits_count( + str_replace_file_tool: StrReplaceFile, temp_work_dir: KaosPath +): + """Edits apply in sequence, so a later edit can target text an earlier edit + introduced, and the reported replacement count reflects what actually ran.""" + file_path = temp_work_dir / "test.txt" + await file_path.write_text("foo") + + result = await str_replace_file_tool( + Params( + path=str(file_path), + edit=[ + Edit(old="foo", new="bar"), + Edit(old="bar", new="baz"), + ], + ) + ) + + assert not result.is_error + assert await file_path.read_text() == "baz" + assert "2 total replacement(s)" in result.message + + +async def test_replace_noop_reports_unchanged_not_missing( + str_replace_file_tool: StrReplaceFile, temp_work_dir: KaosPath +): + """An edit whose old string equals its new string matches but changes + nothing. The error must say the file was left unchanged, not the misleading + "old string was not found" (which would send the model re-reading the file + for a string that is actually present).""" + file_path = temp_work_dir / "test.txt" + original_content = "Hello world!" + await file_path.write_text(original_content) + + result = await str_replace_file_tool( + Params(path=str(file_path), edit=Edit(old="world", new="world")) + ) + + assert result.is_error + assert "left the file unchanged" in result.message + assert "not found" not in result.message + assert await file_path.read_text() == original_content + + +async def test_replace_rejects_empty_old_string( + str_replace_file_tool: StrReplaceFile, temp_work_dir: KaosPath +): + """An empty old string must be rejected. `str.count('')` is never 0 and + `str.replace('', new)` would splice new between every character, so the + occurrence check cannot guard it — reject it explicitly instead.""" + file_path = temp_work_dir / "test.txt" + original_content = "Hello world!" + await file_path.write_text(original_content) + + result = await str_replace_file_tool(Params(path=str(file_path), edit=Edit(old="", new="X"))) + + assert result.is_error + assert "must not be empty" in result.message + assert await file_path.read_text() == original_content