diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 382069e48..4955fb7e4 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -2279,11 +2279,11 @@ def str_replace_tool( # Custom mount paths are resolved by LocalSandbox._resolve_path() with get_file_operation_lock(sandbox, path): content = sandbox.read_file(path) - if not content: - if not old_str: - return "OK" - return f"Error: String to replace not found in file: {requested_path}" - if old_str not in content: + if not old_str: + # A no-op edit. str.replace("", new_str) would insert new_str at + # every character boundary, so this cannot fall through. + return "OK" + if not content or old_str not in content: return f"Error: String to replace not found in file: {requested_path}" if replace_all: content = content.replace(old_str, new_str) diff --git a/backend/tests/test_str_replace_empty_file.py b/backend/tests/test_str_replace_empty_file.py index a68068d0e..ee2df1f6b 100644 --- a/backend/tests/test_str_replace_empty_file.py +++ b/backend/tests/test_str_replace_empty_file.py @@ -1,10 +1,16 @@ -"""str_replace tool behaviour on empty files. +"""str_replace tool behaviour with an empty file or an empty ``old_str``. An empty file used to short-circuit to ``"OK"`` regardless of ``old_str``, so a real substring replacement silently "succeeded" without changing anything and without telling the model the target was missing. The fix only returns ``"OK"`` on an empty file when ``old_str`` is itself empty (a no-op edit); a non-empty ``old_str`` now reports the string was not found. + +The mirror case is an empty ``old_str`` against a *non-empty* file. ``old_str +not in content`` cannot reject ``""`` because ``"" in content`` is always true, +so it reached ``str.replace("", new_str)``, which inserts at every character +boundary and rewrote the file while still returning ``"OK"``. An empty +``old_str`` is now a no-op whatever the file holds. """ from pathlib import Path @@ -28,27 +34,54 @@ def _local_runtime(tmp_path: Path) -> SimpleNamespace: ) -def _str_replace(tmp_path, monkeypatch, *, old_str: str, new_str: str = "x") -> str: +def _str_replace( + tmp_path, + monkeypatch, + *, + old_str: str, + new_str: str = "x", + content: str = "", + replace_all: bool = False, +) -> tuple[str, str]: runtime = _local_runtime(tmp_path) - (tmp_path / "outputs" / "empty.txt").write_text("", encoding="utf-8") + target = tmp_path / "outputs" / "empty.txt" + target.write_text(content, encoding="utf-8") monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: LocalSandbox("t1")) monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None) - return str_replace_tool.func( + result = str_replace_tool.func( runtime=runtime, description="replace in empty file", path="/mnt/user-data/outputs/empty.txt", old_str=old_str, new_str=new_str, + replace_all=replace_all, ) + return result, target.read_text(encoding="utf-8") def test_empty_file_with_non_empty_old_str_reports_not_found(tmp_path, monkeypatch) -> None: - result = _str_replace(tmp_path, monkeypatch, old_str="something") + result, _ = _str_replace(tmp_path, monkeypatch, old_str="something") assert result.startswith("Error: String to replace not found in file") assert "empty.txt" in result def test_empty_file_with_empty_old_str_returns_ok(tmp_path, monkeypatch) -> None: # An empty old_str is a no-op edit and remains a benign "OK" on an empty file. - result = _str_replace(tmp_path, monkeypatch, old_str="") + result, _ = _str_replace(tmp_path, monkeypatch, old_str="") assert result == "OK" + + +def test_non_empty_file_with_empty_old_str_is_a_no_op(tmp_path, monkeypatch) -> None: + # The same no-op contract has to hold once the file has content: str.replace("") + # would otherwise insert new_str at every character boundary. + source = "def main():\n return 1\n" + result, after = _str_replace(tmp_path, monkeypatch, old_str="", new_str="# header\n", content=source) + assert result == "OK" + assert after == source + + +def test_non_empty_file_with_empty_old_str_and_replace_all_is_a_no_op(tmp_path, monkeypatch) -> None: + source = "def main():\n return 1\n" + result, after = _str_replace(tmp_path, monkeypatch, old_str="", new_str="X", content=source, replace_all=True) + assert result == "OK" + assert after == source