fix(middlewares): include file line count in read-before-write block message and clarify read_file range error (#6020)

* fix(middlewares): include file line count in read-before-write block message and clarify read_file range error

* fix(middlewares): address review feedback on line count pluralization and readability

* fix(middlewares): address round-2 review feedback for PR #6020

Stamping bug (_attach_read_mark, ggnnggez item 1):
Originally planned as a standalone issue; folded here since it is the exact mechanism behind the #6019 gate bypass. _attach_read_mark now returns early when message.content is in READ_FILE_NO_CONTENT_RESULTS - {READ_FILE_EMPTY}. In-band errors (invalid range, start_line exceeds file length, etc.) are returned with status='success' by LangChain so the existing message.status == 'error' guard did not catch them.

HTTP code collision [P3] (willem-bd):
Pre-stamp TOOL_META_KEY on the blocked ToolMessage before normalize_tool_result so files with 401, 403, 404, or 500 lines are not misclassified as auth/server errors by _classify_error_text. normalize_tool_message returns early when an existing TOOL_META_KEY entry is found.

Line-count parity (ggnnggez, inline L317):
Replace splitlines() with count_file_lines() in read_file_contract.py using count('\n'), matching LocalSandbox text-mode iteration and the _truncate_read_file_output formula. splitlines() over-counts for files with \f or \u2028 separators (e.g. pdftotext output).

Read hint (ggnnggez, inline _BLOCK_MESSAGE):
For empty files the ranged-read suggestion is omitted. For non-empty files the hint gives a concrete start_line=max(1, N-29), end_line=N.

Constant rename (ggnnggez, read_file_contract.py):
READ_FILE_INVALID_RANGE is the new canonical name; READ_FILE_EMPTY_RANGE kept as a backward-compat alias. _LEGACY_READ_FILE_EMPTY_RANGE removed (unreachable after the #6019 contract update).

Tests:
- TestBlockMessageLineCount parametrized with \f and \u2028 parity cases
- TestStampingGateBugFix: 4 no-content variants, gate-closed integration probe, empty-file-still-stamps assertion
- test_gate_block_on_numeric_line_count_is_recoverable: 401/403/404/500
- test_contract_constants_membership replaced with test_contract_constants_parity_with_localsandbox

---
NOT INCLUDED: ggnnggez item 2 (file length in read_file range error strings). READ_FILE_NO_CONTENT_RESULTS uses exact string membership and is also consumed by build_skill_usage for skill-read detection; making those strings dynamic requires a dedicated contract refactor. Deferred to a follow-up PR.

Note: test_web_fetch_relative_links failures in the full suite are pre-existing on upstream/main and unrelated to this change.

* test: clarify blank ranged reads in the write gate

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
Dan Caldr 2026-10-01 08:40:04 +02:00 committed by GitHub
parent 5b83cb502c
commit 67db3d883c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 257 additions and 16 deletions

View File

@ -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:

View File

@ -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)

View File

@ -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

View File

@ -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"

View File

@ -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

View File

@ -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