From 9056bac95b64ca3e4960aafe48d1ec3935b3bd18 Mon Sep 17 00:00:00 2001 From: Aashish <145881415+aashish254@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:58:09 +0545 Subject: [PATCH] fix(tools): refuse an empty write_file body that would truncate a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handler opened the target in "w" mode without looking at the body, so a call whose content section was lost by a parser cut the file to 0 bytes and still answered exit_code=0 (#6414). Gate the truncating open on the size the file has on disk — the read just above it answers "" for bytes it cannot decode, so an undecodable target would otherwise look empty — and let only an inline-JSON content key that is literally an empty string declare the clear. Also stop turning a null content into the four characters "None", which could be neither refused as a lost body nor honoured as an empty write. --- src/agent_loop.py | 4 +- src/agent_tools/filesystem_tools.py | 45 +++++- tests/test_write_file_empty_body.py | 205 ++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 tests/test_write_file_empty_body.py diff --git a/src/agent_loop.py b/src/agent_loop.py index 178443bf3..b6fa5f29f 100644 --- a/src/agent_loop.py +++ b/src/agent_loop.py @@ -609,7 +609,9 @@ Read a file and return its contents.""", ``` -Write content to a file. First line is the path, rest is the content.""", +Write content to a file. First line is the path, rest is the content. An empty body is +refused when the target already holds data — to clear a file on purpose, send +`{"path": "", "content": ""}` instead.""", "edit_file": """\ ```edit_file diff --git a/src/agent_tools/filesystem_tools.py b/src/agent_tools/filesystem_tools.py index 6a5361ab5..ce9078af7 100644 --- a/src/agent_tools/filesystem_tools.py +++ b/src/agent_tools/filesystem_tools.py @@ -289,12 +289,25 @@ class ReadFileTool: data = data[:MAX_READ_CHARS] + f"\n... [truncated at {MAX_READ_CHARS} chars]" return {"output": data, "exit_code": 0} +class _EmptyBodyWouldTruncate(Exception): + """Raised inside the write thread when an undeclared empty body is about to + replace a file that holds bytes. Carries the size at risk so the caller can be + told what it would have lost (#6414).""" + + def __init__(self, path: str, existing_bytes: int): + super().__init__(path) + self.path = path + self.existing_bytes = existing_bytes + class WriteFileTool: async def execute(self, content: str, ctx: dict) -> dict: from src.tool_execution import _resolve_tool_path, _resolve_search_root, _truncate lines = content.split("\n", 1) raw_path = lines[0].strip() body = lines[1] if len(lines) > 1 else "" + # Only the fenced inline-JSON form can say "this file is meant to be empty": + # the text form's `path\n` and a body a parser dropped look identical here. + declared_clear = False # Decode JSON-object args (the fenced inline-args shape # ```write_file {"path": "...", "content": "..."}```), matching # ReadFileTool above. Without this the whole JSON string becomes the @@ -307,7 +320,17 @@ class WriteFileTool: _a = json.loads(_stripped) if isinstance(_a, dict) and "path" in _a: raw_path = str(_a.get("path", "")).strip() - body = str(_a.get("content", "")) + _content = _a.get("content") + # A `content` key that is literally an empty (or whitespace-only) + # string is the caller declaring the file should be cleared. A + # missing key or a null is what a parser that lost the body leaves + # behind, so neither declares anything. The old + # `str(_a.get("content", ""))` also turned null into the 4 bytes + # "None", which could be neither refused nor honoured. + declared_clear = isinstance(_content, str) and not _content.strip() + body = "" if _content is None else ( + _content if isinstance(_content, str) else str(_content) + ) except (json.JSONDecodeError, TypeError, ValueError): pass try: @@ -322,6 +345,15 @@ class WriteFileTool: old = f.read() except (FileNotFoundError, IsADirectoryError, UnicodeDecodeError, OSError): old = "" + if not body.strip() and not declared_clear: + # Why size on disk rather than `old`: the read above answers "" + # for a file it cannot decode, so a non-UTF-8 target holding real + # bytes looks empty through `old` and would still be truncated. + # Why in this position: open(path, "w") truncates on entry, so a + # check after the write has nothing left to protect. + existing_bytes = os.path.getsize(path) if os.path.isfile(path) else 0 + if existing_bytes > 0: + raise _EmptyBodyWouldTruncate(path, existing_bytes) d = os.path.dirname(path) if d: os.makedirs(d, exist_ok=True) @@ -329,6 +361,17 @@ class WriteFileTool: f.write(body) return old, len(body) old_content, size = await asyncio.to_thread(_write) + except _EmptyBodyWouldTruncate as e: + return { + "error": ( + f"write_file: refused to write an empty body over {e.path} — it holds " + f"{e.existing_bytes} bytes, which the write would have destroyed, so " + f"the file is unchanged. To clear it on purpose, resend with an " + f"explicit empty content: " + f'{{"path": "{raw_path}", "content": ""}}' + ), + "exit_code": 1, + } except PermissionError: return {"error": f"write_file: {path}: permission denied", "exit_code": 1} except OSError as e: diff --git a/tests/test_write_file_empty_body.py b/tests/test_write_file_empty_body.py new file mode 100644 index 000000000..548f4cfa0 --- /dev/null +++ b/tests/test_write_file_empty_body.py @@ -0,0 +1,205 @@ +"""write_file: an empty body must not truncate a file that holds data (#6414). + +The reporter's shape: a model call whose arguments lost their content section +(a #6013-class parser failure) reaches WriteFileTool with an empty body, the +existing file is opened in "w" mode, and the tool answers exit_code=0 with +"Wrote 0 bytes". Each "is refused" test below measures that the bytes at the +path are still there afterwards; each "still works" test guards the write path +this change must not narrow. +""" +import json +import os +import re +import tempfile + +import pytest + +from src import tool_execution as te +from src.agent_tools import ToolBlock +from src.agent_tools.filesystem_tools import EditFileTool, WriteFileTool + +RECIPE = "# Classic banana cake\n\nMash 3 bananas. Bake 180C for 1 hour.\n" + + +@pytest.fixture +def target(): + """A fresh directory under the system temp root, which _tool_path_roots allows.""" + with tempfile.TemporaryDirectory(prefix="odysseus-6414-") as directory: + yield os.path.join(directory, "classic-banana-cake.md") + + +def _seed(path, text=RECIPE): + with open(path, "w", encoding="utf-8") as handle: + handle.write(text) + return text + + +def _read(path): + with open(path, encoding="utf-8") as handle: + return handle.read() + + +def _text_call(path, body=None): + """The documented text form: first line is the path, the rest is the content.""" + return path if body is None else f"{path}\n{body}" + + +def _json_call(path, **content): + """The fenced inline-JSON form, which the handler decodes itself.""" + return json.dumps({"path": path, **content}) + + +# ── The truncation the issue reports ────────────────────────────────────── +@pytest.mark.asyncio +async def test_empty_body_after_the_path_line_is_refused_and_the_file_survives(target): + _seed(target) + res = await WriteFileTool().execute(_text_call(target, ""), {}) + assert res["exit_code"] == 1, res + assert _read(target) == RECIPE + + +@pytest.mark.asyncio +async def test_path_only_call_with_no_content_section_is_refused(target): + """`lines[1] if len(lines) > 1 else ""` has two producers; this is the no-newline one.""" + _seed(target) + res = await WriteFileTool().execute(_text_call(target), {}) + assert res["exit_code"] == 1, res + assert _read(target) == RECIPE + + +@pytest.mark.asyncio +async def test_inline_json_without_a_content_key_is_refused(target): + """A parser that keeps `path` and drops `content` is the reported failure.""" + _seed(target) + res = await WriteFileTool().execute(json.dumps({"path": target}), {}) + assert res["exit_code"] == 1, res + assert _read(target) == RECIPE + + +@pytest.mark.asyncio +async def test_inline_json_null_content_is_refused_and_never_written_as_the_word_none(target): + """`str(_a.get("content", ""))` on a null turns a lost body into the 4 bytes "None".""" + _seed(target) + res = await WriteFileTool().execute(_json_call(target, content=None), {}) + assert res["exit_code"] == 1, res + assert _read(target) == RECIPE + + +@pytest.mark.asyncio +async def test_whitespace_only_body_is_refused(target): + """A body that carries no characters is the same failure with padding left in.""" + _seed(target) + res = await WriteFileTool().execute(_text_call(target, " \n "), {}) + assert res["exit_code"] == 1, res + assert _read(target) == RECIPE + + +@pytest.mark.asyncio +async def test_non_utf8_target_is_refused_on_its_size_not_on_the_decoded_read(target): + """The existing read swallows UnicodeDecodeError and answers "", which would let a + binary or latin-1 file look empty to the guard while holding real bytes.""" + with open(target, "wb") as handle: + handle.write(b"\xc3\xa9\xe8\xaf\xad\xff\xfe\x00binary-ish payload") + before = os.path.getsize(target) + assert before > 0 + res = await WriteFileTool().execute(_text_call(target, ""), {}) + assert res["exit_code"] == 1, res + assert os.path.getsize(target) == before + + +@pytest.mark.asyncio +async def test_refusal_creates_no_extra_files_next_to_the_target(target): + _seed(target) + directory = os.path.dirname(target) + res = await WriteFileTool().execute(_text_call(target, ""), {}) + assert res["exit_code"] == 1, res + assert os.listdir(directory) == [os.path.basename(target)] + + +@pytest.mark.asyncio +async def test_refusal_names_the_byte_count_and_the_explicit_form(target): + _seed(target) + res = await WriteFileTool().execute(_text_call(target, ""), {}) + error = res.get("error", "") + assert str(len(RECIPE)) in error, error + # The caller in a loop has to be able to correct itself in one round. + assert '"content": ""' in error, error + assert "output" not in res, res + + +@pytest.mark.asyncio +async def test_the_resend_the_refusal_prints_actually_clears_the_file(target): + """The guidance is only useful if a caller can paste it back verbatim. This reads + the JSON object out of the refusal and runs it as the next call.""" + _seed(target) + refused = await WriteFileTool().execute(_text_call(target, ""), {}) + resend = re.search(r"\{.*\}", refused["error"], re.S) + assert resend, refused + res = await WriteFileTool().execute(resend.group(0), {}) + assert res["exit_code"] == 0, res + assert os.path.getsize(target) == 0 + + +# ── Deliberate writes this change must keep working ─────────────────────── +@pytest.mark.asyncio +async def test_explicit_empty_content_in_the_json_form_clears_the_file(target): + """"A deliberate empty-file creation can be made explicit" (the issue's own words): + a `content` key that is literally an empty string is a declaration, not a loss.""" + _seed(target) + res = await WriteFileTool().execute(_json_call(target, content=""), {}) + assert res["exit_code"] == 0, res + assert os.path.getsize(target) == 0 + + +@pytest.mark.asyncio +async def test_empty_body_on_a_new_path_still_creates_an_empty_file(target): + res = await WriteFileTool().execute(_text_call(target, ""), {}) + assert res["exit_code"] == 0, res + assert os.path.isfile(target) and os.path.getsize(target) == 0 + + +@pytest.mark.asyncio +async def test_empty_body_over_an_already_empty_file_succeeds(target): + """Nothing is at risk, so the guard has nothing to refuse.""" + _seed(target, "") + res = await WriteFileTool().execute(_text_call(target, ""), {}) + assert res["exit_code"] == 0, res + assert os.path.getsize(target) == 0 + + +@pytest.mark.asyncio +async def test_a_real_body_still_writes_and_reports_a_diff(target): + _seed(target) + replacement = "# Classic banana cake\n\nMash 4 bananas.\n" + res = await WriteFileTool().execute(_text_call(target, replacement), {}) + assert res["exit_code"] == 0, res + assert _read(target) == replacement + assert res["diff"]["added"] == 1 and res["diff"]["removed"] == 1 + + +@pytest.mark.asyncio +async def test_edit_file_remains_an_explicit_way_to_clear_a_file(target): + """The route this change leaves open for a caller that cannot reach the fenced + inline-JSON form: replace the whole content with nothing.""" + _seed(target) + res = await EditFileTool().execute( + json.dumps({"path": target, "old_string": RECIPE, "new_string": ""}), {} + ) + assert res["exit_code"] == 0, res + assert os.path.getsize(target) == 0 + + +# ── The live dispatch path, not just the handler ────────────────────────── +@pytest.mark.asyncio +async def test_execute_tool_block_refuses_a_lost_body_without_touching_the_file(target, monkeypatch): + """#6414 reached the reporter through a parsed model call, so the refusal has to + survive execute_tool_block's wrapping and still report failure upstream.""" + _seed(target) + monkeypatch.setattr(te, "_owner_is_admin", lambda owner: True) + _desc, result = await te.execute_tool_block( + ToolBlock("write_file", _text_call(target, "")), + owner="admin", + security_context=te.NO_TOOL_SECURITY_CONTEXT, + ) + assert result.get("exit_code") == 1, result + assert _read(target) == RECIPE