From 7b27b6adbe35542d0bcaab81d80a9ff153b12b76 Mon Sep 17 00:00:00 2001 From: Osamaali313 <86572800+Osamaali313@users.noreply.github.com> Date: Sun, 14 Jun 2026 19:12:59 +0300 Subject: [PATCH 1/2] fix(tools): fail StrReplaceFile when a multi-edit hunk is unmatched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StrReplaceFile applied every edit with str.replace and then only errored when the *entire* result was unchanged (`content == original_content`). So in a multi-edit call where some edits matched and others did not, the non-matching edits were silently skipped and the tool still reported success ("Applied N edit(s)"). The model believes all hunks landed when some never did — a silent loss of requested changes that is easy to hit when the file has drifted from what the model last read. Apply edits sequentially and validate that each edit's old string is present at its turn (a later edit may legitimately target text an earlier edit introduced). If any edit does not match, fail the whole call without writing, so the edit batch is atomic and the failure is surfaced with the offending edit index. This also corrects the reported replacement count, which was computed against the original content for every edit and therefore wrong for sequential dependent edits (e.g. foo->bar then bar->baz reported 1 instead of 2). Adds tests for the atomic-failure and sequential-dependent-edit cases. --- src/kimi_cli/tools/file/replace.py | 40 ++++++++++++++++------- tests/tools/test_str_replace_file.py | 49 ++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/kimi_cli/tools/file/replace.py b/src/kimi_cli/tools/file/replace.py index 4f551de4f..f76878220 100644 --- a/src/kimi_cli/tools/file/replace.py +++ b/src/kimi_cli/tools/file/replace.py @@ -134,11 +134,37 @@ 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): + 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 + # Defensive: an edit whose old string equals its new string matches + # but changes nothing, so guard the no-op case explicitly. if content == original_content: return ToolError( message="No replacements were made. The old string was not found in the file.", @@ -169,14 +195,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..8d9fa8e15 100644 --- a/tests/tools/test_str_replace_file.py +++ b/tests/tools/test_str_replace_file.py @@ -246,3 +246,52 @@ 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 From 884c6717532db987900b4a5d512be4bf35909a20 Mon Sep 17 00:00:00 2001 From: Osamaali313 <86572800+Osamaali313@users.noreply.github.com> Date: Sun, 14 Jun 2026 22:38:24 +0300 Subject: [PATCH 2/2] fix(tools): reject empty old_string and report no-op edits accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the multi-edit validation: - Empty old_string: `str.count('')` returns `len(content) + 1` (never 0), so the occurrence check could not catch it and `str.replace('', new)` would splice `new` between every character. Reject an empty old string per edit with a clear message. - No-op edits: when every edit matches but the result equals the original (old_string == new_string, or edits that cancel out), the final guard previously reported "old string was not found" — factually wrong and liable to send the model re-reading the file for a string that is present. Report that the edit(s) left the file unchanged instead. Adds tests for both cases. --- src/kimi_cli/tools/file/replace.py | 28 +++++++++++++++++--- tests/tools/test_str_replace_file.py | 38 ++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/kimi_cli/tools/file/replace.py b/src/kimi_cli/tools/file/replace.py index f76878220..122a95146 100644 --- a/src/kimi_cli/tools/file/replace.py +++ b/src/kimi_cli/tools/file/replace.py @@ -142,6 +142,20 @@ async def __call__(self, params: Params) -> ToolReturnValue: # 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 = ( @@ -163,12 +177,18 @@ async def __call__(self, params: Params) -> ToolReturnValue: content = self._apply_edit(content, edit) total_replacements += occurrences if edit.replace_all else 1 - # Defensive: an edit whose old string equals its new string matches - # but changes nothing, so guard the no-op case explicitly. + # 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( diff --git a/tests/tools/test_str_replace_file.py b/tests/tools/test_str_replace_file.py index 8d9fa8e15..4cfb2bef3 100644 --- a/tests/tools/test_str_replace_file.py +++ b/tests/tools/test_str_replace_file.py @@ -295,3 +295,41 @@ async def test_replace_sequential_dependent_edits_count( 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