diff --git a/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py index ddcd025cde..90bf286c4e 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py @@ -50,9 +50,10 @@ from langgraph.prebuilt.tool_node import ToolCallRequest from langgraph.types import Command from deerflow.agents.middlewares.tool_call_args import pair_tool_call_results, rewrite_messages_tool_call_args -from deerflow.agents.middlewares.tool_result_meta import normalize_tool_result, stamp_exception_meta +from deerflow.agents.middlewares.tool_result_meta import TOOL_META_KEY, normalize_tool_result, stamp_exception_meta from deerflow.config.read_before_write_config import ReadBeforeWriteConfig from deerflow.sandbox.exceptions import SandboxAuthorizationError +from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY, READ_FILE_NO_CONTENT_RESULTS, count_file_lines from deerflow.sandbox.tools import ( read_current_file_content, sandbox_authorization_scope, @@ -82,12 +83,7 @@ _ELIDED_PAYLOAD_TEMPLATE = "[payload elided: {chars} chars; this {tool_name} cal # treated as "cannot inspect" — the gate fails open and no mark is stamped. _UNINSPECTABLE_CONTENT_PREFIX = "Error:" -_BLOCK_MESSAGE = ( - "Error: {tool_name} blocked — {path} already exists and you have not read its current version. " - "Any write invalidates earlier reads, so re-read before every modification. " - "Call read_file on it (a ranged read of the relevant section is enough, e.g. the last ~30 lines " - "before an append), check what is already there, then retry." -) +_BLOCK_MESSAGE = "Error: {tool_name} blocked — {path} already exists ({line_desc}) and you have not read its current version. Any write invalidates earlier reads, so re-read before every modification. {read_hint}" # Per-(scope, path) locks serializing gate check + tool execution. Same # WeakValueDictionary pattern as sandbox/file_operation_lock.py, but a @@ -315,13 +311,38 @@ class ReadBeforeWriteMiddleware(AgentMiddleware): if self._latest_mark_hash(request.state, norm_path) == _content_hash(current): return None tool_name = str(tool_call.get("name", "write")) - return ToolMessage( - content=_BLOCK_MESSAGE.format(tool_name=tool_name, path=path), + # count_file_lines matches LocalSandbox.read_file line numbering and the + # truncation-marker formula in _truncate_read_file_output, so the number + # reported here agrees with start_line/end_line the model can pass back. + line_count = count_file_lines(current) + line_desc = f"{line_count} line" if line_count == 1 else f"{line_count} lines" + if line_count == 0: + # Empty file: there is nothing to range-read; keep the hint simple. + read_hint = "Call read_file on it first, check what is already there, then retry." + else: + start = max(1, line_count - 29) + read_hint = f"Call read_file on it first (e.g. start_line={start}, end_line={line_count}), check what is already there, then retry." + blocked = ToolMessage( + content=_BLOCK_MESSAGE.format(tool_name=tool_name, path=path, line_desc=line_desc, read_hint=read_hint), tool_call_id=str(tool_call.get("id", "")), name=tool_name, status="error", - additional_kwargs={WRITE_BLOCK_KEY: {"path": norm_path, "tool": tool_name}}, + additional_kwargs={ + WRITE_BLOCK_KEY: {"path": norm_path, "tool": tool_name}, + # Pre-stamp gate metadata so normalize_tool_result's text heuristics + # cannot misclassify a numeric line count (e.g. 401, 403, 500) as an + # HTTP error code. normalize_tool_message returns early when an + # existing TOOL_META_KEY entry is found (idempotency guard). + TOOL_META_KEY: { + "status": "error", + "error_type": None, + "recoverable_by_model": True, + "recommended_next_action": "try_alternative", + "source": "tool_return", + }, + }, ) + return blocked @staticmethod def _requested_path(request: ToolCallRequest) -> str | None: @@ -384,6 +405,20 @@ class ReadBeforeWriteMiddleware(AgentMiddleware): message = self._extract_tool_message(result) if message is None or message.status == "error": return + # In-band contract errors (invalid range, start_line exceeds file length, etc.) + # are returned with status="success" by LangChain even though the model received + # no real file content. Stamping a hash in that case would open the write gate + # for files the model has never actually read — the root cause of issue #6019. + # READ_FILE_EMPTY is a successful read of an empty file or a valid range + # containing only blank lines. Like any successful ranged read, it stamps + # the current full-file hash and can satisfy the gate. + tool_content = message.content if isinstance(message.content, str) else "" + if tool_content.strip() in READ_FILE_NO_CONTENT_RESULTS - {READ_FILE_EMPTY}: + logger.debug( + "read-before-write mark skipped for %r: in-band no-content result from read_file", + path, + ) + return try: content = self._content_reader(request.runtime, path) except SandboxAuthorizationError: diff --git a/backend/packages/harness/deerflow/sandbox/read_file_contract.py b/backend/packages/harness/deerflow/sandbox/read_file_contract.py index 4c26ac1c01..73c90a8678 100644 --- a/backend/packages/harness/deerflow/sandbox/read_file_contract.py +++ b/backend/packages/harness/deerflow/sandbox/read_file_contract.py @@ -4,14 +4,34 @@ READ_FILE_EMPTY = "(empty)" READ_FILE_START_LINE_EXCEEDS = "(start_line exceeds file length)" READ_FILE_INVALID_START_LINE = "(start_line must be >= 1)" READ_FILE_INVALID_END_LINE = "(end_line must be >= 1)" -READ_FILE_EMPTY_RANGE = "(start_line > end_line — no lines in range)" +READ_FILE_INVALID_RANGE = "(invalid line range: start_line must be <= end_line)" +# Backward-compat alias — callers that imported READ_FILE_EMPTY_RANGE by name continue to work. +READ_FILE_EMPTY_RANGE = READ_FILE_INVALID_RANGE READ_FILE_NO_CONTENT_RESULTS = frozenset( { READ_FILE_EMPTY, READ_FILE_START_LINE_EXCEEDS, READ_FILE_INVALID_START_LINE, READ_FILE_INVALID_END_LINE, - READ_FILE_EMPTY_RANGE, + READ_FILE_INVALID_RANGE, } ) READ_FILE_TRUNCATION_PREFIX = "... [truncated:" + + +def count_file_lines(content: str) -> int: + """Count file lines as LocalSandbox and read_file's truncation marker see them. + + Uses ``count("\\n") + (1 if not endswith("\\n") else 0)`` — matching Python's + text-mode line iteration used by ``LocalSandbox.read_file`` and the formula in + ``_truncate_read_file_output``. This ensures the line count shown in the gate + block message agrees with the ``start_line``/``end_line`` values the model can + pass back to ``read_file``. + + Unlike ``splitlines()``, this function does not treat ``\\f``, ``\\v``, ``\\x85``, + ``\\u2028``, or ``\\u2029`` as line separators — consistent with LocalSandbox's + text-mode iteration where those characters are just regular content. + """ + if not content: + return 0 + return content.count("\n") + (1 if not content.endswith("\n") else 0) diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index af297cab01..fe8eea1930 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -42,8 +42,8 @@ from deerflow.sandbox.overwrite import unwrap_sandbox from deerflow.sandbox.path_patterns import build_output_mask_pattern, normalize_mask_tail, replace_output_path_matches from deerflow.sandbox.read_file_contract import ( READ_FILE_EMPTY, - READ_FILE_EMPTY_RANGE, READ_FILE_INVALID_END_LINE, + READ_FILE_INVALID_RANGE, READ_FILE_INVALID_START_LINE, READ_FILE_START_LINE_EXCEEDS, READ_FILE_TRUNCATION_PREFIX, @@ -2811,7 +2811,7 @@ def read_file_tool( if end_line is not None and end_line < 1: return READ_FILE_INVALID_END_LINE if end_line is not None and effective_start > end_line: - return READ_FILE_EMPTY_RANGE + return READ_FILE_INVALID_RANGE requested_path = path use_line_range = start_line is not None or end_line is not None diff --git a/backend/tests/test_read_before_write_middleware.py b/backend/tests/test_read_before_write_middleware.py index a4ebe9d47b..b84aac41c5 100644 --- a/backend/tests/test_read_before_write_middleware.py +++ b/backend/tests/test_read_before_write_middleware.py @@ -857,3 +857,165 @@ def test_lock_scope_resolves_overwrite_wrapped_sandbox(): runtime=MagicMock(context={}), ) assert ReadBeforeWriteMiddleware._lock_scope(req) == "sb-fork-lock" + + +@pytest.mark.parametrize( + "tool,content,expected_desc", + [ + ("write_file", "line1\nline2\nline3", "3 lines"), + ("write_file", "line1\n", "1 line"), + ("write_file", "line1", "1 line"), + ("write_file", "", "0 lines"), + ("str_replace", "line1\nline2\n", "2 lines"), + # Exotic separators: \f and \u2028 are NOT line boundaries in LocalSandbox + # (Python text-mode iterates only \n/\r\n/\r), so count must match LocalSandbox. + ("write_file", "x\fx\n", "1 line"), + ("write_file", "x\u2028x\n", "1 line"), + ], +) +def test_block_message_line_desc(tool, content, expected_desc): + PATH = "/mnt/user-data/outputs/report.md" + args = {"description": "d", "path": PATH, "content": "v2"} if tool == "write_file" else {"description": "d", "path": PATH, "old_str": "a", "new_str": "b"} + mw = _middleware({PATH: content}) + result = mw.wrap_tool_call(_make_request(tool, args), MagicMock()) + assert result.status == "error" + assert f"{PATH} already exists ({expected_desc}) and you have not read" in result.content + + +@pytest.mark.parametrize("line_count", [401, 403, 404, 500]) +def test_gate_block_on_numeric_line_count_is_recoverable(line_count): + """Files with 401/403/404/500 lines must not be misclassified as HTTP error codes.""" + from deerflow.agents.middlewares.tool_result_meta import TOOL_META_KEY + + PATH = "/mnt/user-data/outputs/report.md" + # Build a file with exactly `line_count` lines (no trailing newline). + content = "\n" * (line_count - 1) + "x" + mw = _middleware({PATH: content}) + result = mw.wrap_tool_call( + _make_request("write_file", {"description": "d", "path": PATH, "content": "v2"}), + MagicMock(), + ) + assert result.status == "error" + meta = (result.additional_kwargs or {}).get(TOOL_META_KEY) + assert meta is not None, f"gate block on {line_count}-line file must carry deerflow_tool_meta" + assert meta["recoverable_by_model"] is True, f"{line_count}-line file must not be classified as a fatal auth/server error" + assert meta["recommended_next_action"] != "stop", f"{line_count}-line gate block must not tell the agent to stop" + + +class TestStampingGateBugFix: + """In-band errors from read_file must not stamp a read mark (issue #6019).""" + + PATH = "/mnt/user-data/outputs/report.md" + + @pytest.mark.parametrize( + "no_content_result", + [ + pytest.param("READ_FILE_EMPTY_RANGE", id="empty_range"), + pytest.param("READ_FILE_START_LINE_EXCEEDS", id="start_exceeds"), + pytest.param("READ_FILE_INVALID_START_LINE", id="invalid_start"), + pytest.param("READ_FILE_INVALID_END_LINE", id="invalid_end"), + ], + ) + def test_in_band_error_read_does_not_stamp_mark(self, no_content_result): + from deerflow.agents.middlewares.read_before_write_middleware import READ_MARK_KEY + from deerflow.sandbox import read_file_contract as c + + result_string = getattr(c, no_content_result) + mw = _middleware({self.PATH: "a\nb\nc\n"}) + req = _make_request("read_file", {"description": "d", "path": self.PATH}) + res = mw.wrap_tool_call( + req, + lambda r: ToolMessage(content=result_string, tool_call_id="call-1", name="read_file"), + ) + assert READ_MARK_KEY not in res.additional_kwargs, f"in-band error '{result_string}' must not stamp a read mark" + + def test_in_band_error_read_leaves_gate_closed(self): + """A write after an in-band-error read must still be blocked.""" + from deerflow.agents.middlewares.read_before_write_middleware import READ_MARK_KEY + from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY_RANGE + + mw = _middleware({self.PATH: "a\nb\nc\n"}) + req = _make_request("read_file", {"description": "d", "path": self.PATH}) + read_res = mw.wrap_tool_call( + req, + lambda r: ToolMessage(content=READ_FILE_EMPTY_RANGE, tool_call_id="call-1", name="read_file"), + ) + assert READ_MARK_KEY not in read_res.additional_kwargs + write_req = _make_request( + "write_file", + {"description": "d", "path": self.PATH, "content": "x"}, + messages=[read_res], + ) + handler = MagicMock(return_value=ToolMessage(content="OK", tool_call_id="call-1", name="write_file")) + result = mw.wrap_tool_call(write_req, handler) + handler.assert_not_called() + assert result.status == "error" + + @pytest.mark.parametrize( + "content,line_range", + [ + pytest.param("a\n\nb\n", {"start_line": 2, "end_line": 2}, id="both_bounds"), + pytest.param("a\n\n", {"start_line": 2}, id="start_only"), + pytest.param("\nb\n", {"end_line": 1}, id="end_only"), + ], + ) + def test_blank_line_range_stamps_full_hash_and_allows_write(self, tmp_path, monkeypatch, content, line_range): + """Valid blank ranges satisfy the gate like other successful ranged reads.""" + from types import SimpleNamespace + + from deerflow.agents.middlewares.read_before_write_middleware import READ_MARK_KEY, ReadBeforeWriteMiddleware + from deerflow.sandbox import tools as sandbox_tools + from deerflow.sandbox.local.local_sandbox import LocalSandbox + from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY + + outputs = tmp_path / "outputs" + outputs.mkdir() + (outputs / "report.md").write_text(content, encoding="utf-8") + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local:t-test"}, "thread_data": {"outputs_path": str(outputs)}}, + context={"thread_id": "t-test"}, + ) + monkeypatch.setattr(sandbox_tools, "ensure_sandbox_initialized", lambda runtime: LocalSandbox("t-test")) + monkeypatch.setattr(sandbox_tools, "ensure_thread_directories_exist", lambda runtime: None) + mw = ReadBeforeWriteMiddleware() + read_req = ToolCallRequest( + tool_call={"name": "read_file", "args": {"path": self.PATH, **line_range}, "id": "read-blank"}, + tool=None, + state=runtime.state, + runtime=runtime, + ) + + def read_handler(request): + return ToolMessage( + content=sandbox_tools.read_file_tool.func(runtime=request.runtime, **request.tool_call["args"]), + tool_call_id=request.tool_call["id"], + name="read_file", + ) + + read_res = mw.wrap_tool_call(read_req, read_handler) + assert read_res.content == READ_FILE_EMPTY + assert read_res.additional_kwargs[READ_MARK_KEY] == {"path": self.PATH, "hash": _sha(content)} + + write_req = ToolCallRequest( + tool_call={"name": "write_file", "args": {"path": self.PATH, "content": "updated"}, "id": "write-after-blank"}, + tool=None, + state={**runtime.state, "messages": [read_res]}, + runtime=runtime, + ) + handler = MagicMock(return_value=ToolMessage(content="OK", tool_call_id="write-after-blank", name="write_file")) + result = mw.wrap_tool_call(write_req, handler) + handler.assert_called_once_with(write_req) + assert result.status == "success" + + def test_empty_file_read_still_stamps_mark(self): + """READ_FILE_EMPTY (genuine empty-file read) must still stamp a mark.""" + from deerflow.agents.middlewares.read_before_write_middleware import READ_MARK_KEY + from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY + + mw = _middleware({self.PATH: ""}) + req = _make_request("read_file", {"description": "d", "path": self.PATH}) + res = mw.wrap_tool_call( + req, + lambda r: ToolMessage(content=READ_FILE_EMPTY, tool_call_id="call-1", name="read_file"), + ) + assert READ_MARK_KEY in res.additional_kwargs, "a full read of a genuinely empty file must stamp a read mark" diff --git a/backend/tests/test_read_file_line_range.py b/backend/tests/test_read_file_line_range.py index a29654092a..ccae1295fc 100644 --- a/backend/tests/test_read_file_line_range.py +++ b/backend/tests/test_read_file_line_range.py @@ -67,8 +67,10 @@ def test_start_line_negative_returns_clean_error(tmp_path, monkeypatch) -> None: def test_start_line_greater_than_end_line_returns_clean_error(tmp_path, monkeypatch) -> None: + from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY_RANGE + result = _read(tmp_path, monkeypatch, start_line=4, end_line=2) - assert "start_line > end_line" in result + assert result == READ_FILE_EMPTY_RANGE # No garbage slice content leaked into the error. assert "line4" not in result @@ -99,3 +101,23 @@ def test_only_end_line_negative_returns_clean_error(tmp_path, monkeypatch) -> No def test_end_line_past_eof_clamps_to_last_line(tmp_path, monkeypatch) -> None: result = _read(tmp_path, monkeypatch, end_line=99) assert result == _FIVE_LINES + + +def test_contract_constants_parity_with_localsandbox() -> None: + """count_file_lines must agree with LocalSandbox line numbering and all no-content + strings must remain in READ_FILE_NO_CONTENT_RESULTS.""" + from deerflow.sandbox.read_file_contract import ( + READ_FILE_INVALID_RANGE, + READ_FILE_NO_CONTENT_RESULTS, + count_file_lines, + ) + + assert READ_FILE_INVALID_RANGE in READ_FILE_NO_CONTENT_RESULTS + + # Parity with LocalSandbox.read_file (Python text-mode, universal \n only): + assert count_file_lines("") == 0 + assert count_file_lines("line1\n") == 1 + assert count_file_lines("line1\nline2\n") == 2 + assert count_file_lines("line1\nline2") == 2 # no trailing newline + assert count_file_lines("page\fend\n") == 1 # \f not a line separator + assert count_file_lines("x\u2028y\n") == 1 # \u2028 not a line separator diff --git a/backend/tests/test_read_file_tool_binary.py b/backend/tests/test_read_file_tool_binary.py index ff7a6caac1..826962c9b4 100644 --- a/backend/tests/test_read_file_tool_binary.py +++ b/backend/tests/test_read_file_tool_binary.py @@ -163,6 +163,8 @@ def test_read_file_tool_passes_open_ended_ranges_into_sandbox(monkeypatch) -> No def test_read_file_tool_validates_range_order(monkeypatch) -> None: + from deerflow.sandbox.read_file_contract import READ_FILE_EMPTY_RANGE + runtime = SimpleNamespace(state={"sandbox": {"sandbox_id": "aio:test"}}, context={"thread_id": "t1"}) monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: object()) @@ -174,4 +176,4 @@ def test_read_file_tool_validates_range_order(monkeypatch) -> None: end_line=10, ) - assert "start_line > end_line" in result + assert result == READ_FILE_EMPTY_RANGE