diff --git a/emrg/server/daemon.py b/emrg/server/daemon.py index 9c59c82d..526eb20d 100644 --- a/emrg/server/daemon.py +++ b/emrg/server/daemon.py @@ -194,6 +194,12 @@ def resolve_client_tier(session, req_sandbox: str | None = None) -> str: #: roots — and two hand-written unions would be two chances to drift apart. _ROOT_CONSUMING_TOOLS = SHELL_TOOL_NAMES | {"write", "edit"} +#: The file tools whose caller has to be nameable, because one of them decides +#: whether another may replace a file's whole content (issue #2652). ``read`` is +#: here and not in ``_ROOT_CONSUMING_TOOLS`` on purpose: it consumes no root, and +#: adding it there would hand a read the write tier's boundary. +_CALLER_IDENTIFIED_TOOLS = {"read", "write", "edit"} + #: The tools a **host conversation** is offered, by name (issue #2465). #: #: This is a whitelist, and naming a tool is the only way to offer it: a tool @@ -4318,6 +4324,17 @@ def _inject_tool_arguments( tier is resolved from ``tasks.yml`` into the request and never written onto the session, so every child such a task spawned ran unconfined at ``policy.DEFAULT_MODE`` while the task body itself was confined. + * **who is asking is a fact, not a field.** ``read``, ``write`` and + ``edit`` are told the session they belong to, under ``_session_id``, + injected from the session and therefore replacing any value the model + put in the call (issue #2652). One of the three decides something about + another: a whole-file ``write`` is answered only when this session has + been shown every line of the file that is there now, and the reading + that showed them was filed under this name. The model is the party that + guard is about, so an identity it chose is the one input the guard + cannot take from it — and a reading filed under a name nobody injected + is a reading a write can never find, which is why ``read`` is in this + class rather than left anonymous. ``workspace`` is a new key for the shell tools and the old executor ignores it (it reads ``command``/``timeout``/``workdir``/``sandbox`` @@ -4338,6 +4355,16 @@ def _inject_tool_arguments( :param req: the request carrying the task's configured tier. """ cwd = str(session.cwd) + if tc_name in _CALLER_IDENTIFIED_TOOLS: + # **Who is asking**, for the one guard that has to know: `write` + # replaces a file's whole content, and it refuses while a read has not + # shown the caller that content (issue #2652 — measured by writing a + # 5,000-line file back from a capped read of it: 1,002 lines survived, + # and the call reported `Updated`). Injected unconditionally, so a + # value the model put in the call is **replaced**: the model is the + # party that guard is about, and an identity it chose is the one fact + # the guard cannot take from it. + args["_session_id"] = session.session_id # Two classes, and what separates them is what the value decides. A shell # tool runs in this directory and may write under it, so the model cannot # name it; a read-only discovery tool does neither, so its directory is a diff --git a/emrg/tools/edit_tool.py b/emrg/tools/edit_tool.py index 5682b577..c47ce3a1 100644 --- a/emrg/tools/edit_tool.py +++ b/emrg/tools/edit_tool.py @@ -9,6 +9,7 @@ from emrg.sandbox.fence import file_refusal from emrg.sandbox.policy import grants_from_arguments, resolve_policy from emrg.server.tool_types import ToolDefinition, ToolResult +from emrg.tools import read_receipts from emrg.tools.base import ( ToolExecutor, boolean_argument, @@ -126,7 +127,8 @@ async def execute(self, arguments: dict) -> ToolResult: # name the directory alone, so the rest reached `open` and never returned # (measured 2026-10-10, `cyc20261010-215146`: `edit` on a `mkfifo` named pipe did # not return within 10 s, in its own process, while a regular file's edit did). - special = special_file_kind(path.stat().st_mode) + before = path.stat() + special = special_file_kind(before.st_mode) if special is not None: return ToolResult( name="edit", content=special_file_refusal(path, special), error=True @@ -190,4 +192,49 @@ async def execute(self, arguments: dict) -> ToolResult: ) desc = f"{count} replacements" if replace_all else "1 replacement" + + # ── The receipt this replacement produced ── + # + # `edit` is a writer, so it moves the file's version out from under any + # receipt filed for the old one — and before this rule it left the receipt + # stale rather than moving it, which broke the loop a code-writing agent + # lives in: write a file, edit it, then rewrite it whole, and the rewrite + # was refused with "none of them has been shown to this session" about text + # the session had itself just written (measured 2026-10-30, the review that + # found it; the memory-index shape hits it — write the index, three edits, + # the whole-file rewrite refused). + # + # It is recorded **only when the version being replaced had been shown in + # full**. Then every byte of the new version is known to the caller too: the + # text it is the old text with one named string replaced. A file nobody read, + # or one read only in part, keeps no receipt and therefore keeps the refusal + # — which is the direction that cannot destroy anything. + previous = read_receipts.coverage( + path, + size=before.st_size, + mtime_ns=before.st_mtime_ns, + session=arguments.get("_session_id"), + ) + if previous is not None and previous[0] >= previous[1]: + try: + after = path.stat() + except OSError: + after = None + if after is not None: + # Counted the way the read and write tools count, so the number this + # receipt holds is the number those tools would print back. + written_lines = len(new_content.split("\n")) + if written_lines and new_content.endswith("\n"): + written_lines -= 1 + read_receipts.record( + path, + size=after.st_size, + mtime_ns=after.st_mtime_ns, + start_line=1, + end_line=written_lines, + total_lines=written_lines, + session=arguments.get("_session_id"), + digest=read_receipts.digest_of(path), + ) + return ToolResult(name="edit", content=f"Made {desc} in {path}") diff --git a/emrg/tools/read_receipts.py b/emrg/tools/read_receipts.py new file mode 100644 index 00000000..7cc0a64c --- /dev/null +++ b/emrg/tools/read_receipts.py @@ -0,0 +1,238 @@ +"""What a read has shown, so a write cannot replace a file with less of it. + +The class this exists for +------------------------- +``read`` answers a **window**, not a file: it stops at ``DEFAULT_MAX_LINES`` +unless the caller asks for more, and it says so in its own report. ``write`` +replaces a file's whole content. Nothing connected the two, so a capped read +followed by a write of what that read returned destroyed every line past the +cap — and the write reported success (issue #2652). + +Measured by calling the two tools directly on a 5,000-line file, in a temporary +directory, with no daemon and no session state: + + BEFORE: 5000 lines, 193893 chars + READ: 1002 lines returned, 44967 chars, error=False + WRITE: Updated ... (44967 characters), error=False + AFTER: 1002 lines, 44967 chars + +The read tool's truncation marker — its own sentence saying the reading is +partial — was written into the file as content, so what survived did not even +end at a clean prefix of the original. + +What a receipt is +----------------- +A fact about a **file version**, recorded by every file read: the version (size, +``mtime_ns`` and — once the whole file has been shown — a digest of its bytes), +how many lines from the start of the file that session has been shown, and how +many the file holds. ``write`` asks for one before it overwrites an existing +file, and refuses while ``shown`` is short of ``total`` — naming both numbers, so +the caller learns what it was about to remove. + +Four properties are deliberate, and each closes a way the guard could lie: + +* **Version-keyed, and the version is the bytes.** A file that changed after the + read is not the file that was shown, so the receipt expires by itself instead + of authorising a write against content nobody has seen. Size and ``mtime_ns`` + are the cheap half of that key and the digest is the half that cannot be + spoofed by an unchanged pair of numbers: a whole-file replacement that preserves + the length *and* the modification time is ordinary, not exotic — ``cp -p``, + ``rsync -a``, ``tar -x`` and ``git checkout`` all restore the old timestamp onto + new bytes — and a receipt keyed on the two numbers alone answers that state as + though the file had not moved (measured 2026-10-30 on the branch this module + was reviewed on: ``'AAAAAAAAAA'`` read, ``'BBBBBBBBBB'`` written back at the same + length and the same ``mtime_ns``, whole-file write answered, and the unread + content destroyed). +* **A contiguous prefix from line 1.** Reading a file in chunks extends the + receipt when the next chunk starts where the last one ended, so the ordinary + way of reading a long file does reach ``total``. A window that starts past the + beginning leaves the count alone: it understates what was seen rather than + overstating it, which is the only safe direction for a guard whose wrong answer + destroys text. +* **A digest only when the whole file was shown.** A digest of a partial reading + describes bytes the file no longer has as a whole, so recording one would + vouch for a version nobody saw. Partial coverage therefore carries no digest, + and :func:`content_matches` — which is asked only about a version the receipt + already claims to have covered completely — answers ``False`` rather than + falling back on the numbers. +* **Missing is a refusal, never a pass.** No receipt and a short receipt are + different states with the same consequence, so the one function that answers + cannot return a value that reads like "fine". + +Scope, stated rather than implied +--------------------------------- +A receipt is keyed by ``(session, resolved path)``. The session id is the one the +daemon injects (``_session_id`` in +:meth:`emrg.server.daemon.EmgServer._inject_tool_arguments`), and it **replaces** +whatever the model put in the call: the model is the party this guard is about, +so an identity it chose is the one fact the guard cannot take from it. + +A call carrying no identity at all — an in-process caller, a test — shares one +bucket. That is the honest limit of a guard that cannot name its caller, and it +is that way round on purpose: the alternative is a guard whose protection depends +on which caller happens to be asking. + +``edit`` is not a subject here, but it is a **writer**, and the two tools share +one receipt for a reason. Its ``old_string`` must already be in the file before it +changes anything, so it can only remove text the caller has shown it. What it +must not do is leave a stale receipt behind: before this rule, a session that +wrote a file and then edited it could no longer rewrite the file whole, because +the edit had moved the version out from under the receipt the write had filed +(measured 2026-10-30, same review). :func:`emrg.tools.edit_tool.EditTool` now +re-records the receipt for the version it produced, and only when the version it +replaced had been completely shown — so a file the session authored stays +writable, and a file nobody read stays refused. +""" + +from __future__ import annotations + +import hashlib +import threading +from pathlib import Path + +from emrg.tools.base import special_file_kind + +#: How many receipts are kept. Each is four integers, a digest and a path, so this +#: is a few hundred kilobytes at worst; past it the oldest is dropped first, which +#: costs a caller one extra read and never a wrong answer (a missing receipt is a +#: refusal). +MAX_RECEIPTS = 4096 + +_lock = threading.Lock() + +#: ``(session, resolved path) -> (size, mtime_ns, shown_lines, total_lines, digest)``. +#: Insertion-ordered, so the eviction below takes the oldest without sorting. +_receipts: dict[tuple[str, str], tuple[int, int, int, int, str | None]] = {} + + +def _key(session: str | None, path: Path) -> tuple[str, str]: + """The bucket a receipt lives in: who was shown it, and which file.""" + return (session or "", str(path)) + + +def digest_of(path: Path) -> str | None: + """A digest of the bytes on disk, or ``None`` when they cannot be had. + + ``None`` is a refusal everywhere it is used, never "unchanged": a file this + guard cannot read is one it cannot vouch for, and the two states must not + collapse into each other. + + The subject's kind is asked first, for the reason every opener in this tree + asks it: `open` on a FIFO blocks until a peer appears, and this runs on the + daemon's event loop. + """ + try: + info = path.stat() + except OSError: + return None + if special_file_kind(info.st_mode) is not None: + return None + try: + data = path.read_bytes() + except OSError: + return None + return hashlib.sha256(data).hexdigest() + + +def record( + path: Path, + *, + size: int, + mtime_ns: int, + start_line: int, + end_line: int, + total_lines: int, + session: str | None = None, + digest: str | None = None, +) -> None: + """Note the lines of `path` at this version that a read has just shown. + + The stored count is a **contiguous prefix from line 1**: the read that starts + at line 1 sets it, a later read that begins exactly where the last one ended + extends it, and a window that begins anywhere else leaves it where it was. A + window in the middle of a file therefore never counts as showing the file, + which is the failure this module exists to prevent. + + `digest` is stored **only when coverage is complete**: a digest of a partial + reading does not describe the file as a whole, so keeping it would let a + later write be judged against bytes nobody was shown. + """ + key = _key(session, path) + with _lock: + # Re-inserted rather than overwritten so insertion order keeps meaning + # "least recently recorded", which is what the eviction rule reads. + previous = _receipts.pop(key, None) + shown = end_line if start_line <= 1 else 0 + if previous is not None: + size_then, mtime_then, shown_then, _total_then, _digest_then = previous + if size_then == size and mtime_then == mtime_ns: + if start_line <= shown_then + 1: + shown = max(shown, end_line) + else: + shown = max(shown, shown_then) + keeps_digest = digest if shown >= total_lines else None + _receipts[key] = (size, mtime_ns, shown, total_lines, keeps_digest) + while len(_receipts) > MAX_RECEIPTS: + _receipts.pop(next(iter(_receipts))) + + +def coverage( + path: Path, + *, + size: int, + mtime_ns: int, + session: str | None = None, +) -> tuple[int, int] | None: + """``(lines shown, lines held)`` for this exact version, or ``None``. + + ``None`` means "nothing has been read of this version" — never "the file is + fine". The two are the whole distinction this module exists to keep: a caller + that has been shown the file and one that has not are different states, and + only the first may replace it. A returned pair whose first number is below its + second is the third state, and it is the one the reproduction above lives in. + """ + with _lock: + receipt = _receipts.get(_key(session, path)) + if receipt is None: + return None + size_then, mtime_then, shown_then, total_then, _digest_then = receipt + if size_then != size or mtime_then != mtime_ns: + return None + return shown_then, total_then + + +def content_matches( + path: Path, + *, + size: int, + mtime_ns: int, + session: str | None = None, +) -> bool: + """Whether the bytes on disk are the bytes this version's receipt was filed on. + + Asked only about a version whose coverage is already complete, and it answers + **no** in every case it cannot decide: no receipt, a different version, a + receipt that carries no digest (so the bytes were never recorded), an + unreadable subject, or a digest that differs. The one answer that permits a + whole-file replacement is a digest that matches. + """ + with _lock: + receipt = _receipts.get(_key(session, path)) + if receipt is None: + return False + size_then, mtime_then, _shown_then, _total_then, digest_then = receipt + if size_then != size or mtime_then != mtime_ns or digest_then is None: + return False + return digest_of(path) == digest_then + + +def forget(path: Path, *, session: str | None = None) -> None: + """Drop a receipt. Used when a write makes the version it described obsolete.""" + with _lock: + _receipts.pop(_key(session, path), None) + + +def clear() -> None: + """Forget every receipt. For tests, which must not inherit another's reads.""" + with _lock: + _receipts.clear() diff --git a/emrg/tools/read_tool.py b/emrg/tools/read_tool.py index 965084a2..bd334667 100644 --- a/emrg/tools/read_tool.py +++ b/emrg/tools/read_tool.py @@ -11,6 +11,7 @@ from pathlib import Path from emrg.server.tool_types import ToolDefinition, ToolResult +from emrg.tools import read_receipts from emrg.tools.base import ToolExecutor, count_argument, special_file_kind, special_file_refusal logger = logging.getLogger(__name__) @@ -415,4 +416,43 @@ async def execute(self, arguments: dict) -> ToolResult: if cap_note is not None: result_lines.append(cap_note) + # ── Say how much of the file this reading showed ── + # + # Every file read records its coverage, not only a complete one, because + # the caller of this record needs to name what is missing: a write that + # would replace a file with less of it is refused, and "4 000 of 5 000 + # lines have been shown" is the sentence that makes the refusal + # actionable. A reading that starts past line 1, or resumes a line from a + # byte offset, records no prefix from the start — it shows lines 2..n and + # the first one only partly, so counting it as line 1 would overstate + # what the caller has seen (issue #2652; the reproduction is the + # docstring of `emrg/tools/read_receipts.py`). + covered_from = start_line if start_line_byte_offset == 0 else start_line + 1 + try: + stat = path.stat() + except OSError: + # A file that vanished between the read and here cannot be + # overwritten either, so there is nothing for the receipt to protect. + stat = None + # The digest is taken whenever this reading reaches the end of the file, + # because it describes the bytes on disk — and `record` keeps it only if the + # merged coverage then covers the whole file, so a window that stops short + # of line 1 (or of the last line) drops it rather than vouching for a + # version nobody saw. Tying this to "the call started at line 1" instead + # starved the ordinary way of reading a long file: the second, continuing + # call does not start at line 1, yet it is the call that completes the + # coverage and the only one that could have filed the digest. + digest = read_receipts.digest_of(path) if end >= total_lines else None + if stat is not None: + read_receipts.record( + path, + size=stat.st_size, + mtime_ns=stat.st_mtime_ns, + start_line=covered_from, + end_line=end, + total_lines=total_lines, + session=arguments.get("_session_id"), + digest=digest, + ) + return ToolResult(name="read", content="\n".join(result_lines)) diff --git a/emrg/tools/write_tool.py b/emrg/tools/write_tool.py index 91f2aeda..f36ae0ea 100644 --- a/emrg/tools/write_tool.py +++ b/emrg/tools/write_tool.py @@ -9,6 +9,7 @@ from emrg.sandbox.fence import file_refusal from emrg.sandbox.policy import grants_from_arguments, resolve_policy from emrg.server.tool_types import ToolDefinition, ToolResult +from emrg.tools import read_receipts from emrg.tools.base import ToolExecutor, special_file_kind, special_file_refusal from emrg.tools.file_policy import resolve_file_target @@ -17,6 +18,84 @@ MAX_WRITE_SIZE = 10 * 1024 * 1024 # 10 MB safety limit +def _lines_held(path: Path) -> int | None: + """How many lines `path` holds, by the read tool's own rule, or None. + + The rule is the read tool's rather than a fresh one: a file ending in a + newline splits into one element more than it has lines, and counting that + empty tail made every terminated file read as one line longer than it is + (`emrg/tools/read_tool.py`, the paragraph above `total_lines`). The refusal + below quotes a number back to the caller, so it has to be the number the + caller was shown. + """ + try: + info = path.stat() + except OSError: + return None + # An open is not a read: on a FIFO it blocks until a peer appears, and this + # runs on the daemon's event loop. `execute` has already refused a non-regular + # subject at this path, and the kind is asked again here rather than leaning on + # that — the path can be replaced between the two stats, and a refusal is worth + # less than the daemon it would block. + if special_file_kind(info.st_mode) is not None: + return None + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + # A binary or unreadable subject has no line count to state; the refusal + # falls back to its size rather than inventing one. + return None + lines = text.split("\n") + if lines and lines[-1] == "": + lines.pop() + return len(lines) + + +def _blind_overwrite_refusal( + path: Path, + size: int, + coverage: tuple[int, int] | None, +) -> str: + """The refusal a whole-file replacement earns when the file was not read. + + It names **both** numbers — what the file holds and what has been shown — + because the caller's next action depends on the gap between them, and a + refusal that said only "read it first" would leave the size of the loss + unstated. + """ + held = _lines_held(path) + holds = ( + f"{held:,} lines ({size:,} bytes)" + if held is not None + else f"{size:,} bytes" + ) + if coverage is None: + seen = "none of them has been shown to this session" + else: + shown, total = coverage + seen = ( + f"only {shown:,} of those {total:,} lines have been shown to this " + f"session" + ) + return ( + f"Error: {path} already holds {holds} and this call would replace all of " + f"it, but {seen}.\n\n" + f"A write that replaces content the caller has not read is refused rather " + f"than answered: what it removes is invisible to whoever asked for it, and " + f"the loss is silent — the file is simply shorter, and this tool reports " + f"success.\n\n" + f"Do one of these:\n" + f" - read the file first. `read` returns at most 1000 lines per call and " + f"says so with a `truncated` line; a longer file takes repeated calls from " + f"where the last one stopped, and the refusal stops once the whole file has " + f"been shown;\n" + f" - use `edit` instead, which changes only the text you quote and is the " + f"right tool for a large file.\n\n" + f"Creating a new file needs nothing, and so does replacing a file this " + f"session has just written." + ) + + class WriteTool(ToolExecutor): """Write content to a file, creating parent directories as needed.""" @@ -98,22 +177,109 @@ async def execute(self, arguments: dict) -> ToolResult: return ToolResult(name="write", content=f"Error creating directory: {e}", error=True) existed = path.exists() + before: os.stat_result | None = None if existed: + before = path.stat() # An existing subject has a kind, and only a regular file can be written: # `write_text` on a FIFO blocks at the open until a reader appears (measured # 2026-10-10, `cyc20261010-215146`: no return within 10 s in its own # process, while the same call against a regular file returned). A path that # does not exist yet has no kind to judge, and creating it is the call's job. - special = special_file_kind(path.stat().st_mode) + special = special_file_kind(before.st_mode) if special is not None: return ToolResult( name="write", content=special_file_refusal(path, special), error=True ) + + # ── A whole-file replacement of content nobody has read ── + # + # `write` replaces every byte of its subject, so the one fact it needs + # is whether the caller has been shown them. Nothing asked that + # question, and the cost was measured: a 5,000-line file read through + # the read tool's default cap, written back with what that read + # returned, came out at 1,002 lines — a call that reported `Updated` + # (issue #2652). Every number below comes from the file on disk now, + # not from anything the caller supplied. + # + # The guard is keyed on the version that is actually there: a receipt + # describing an older version does not match, so a file that changed + # after the read is read again rather than overwritten on the strength + # of a reading about different content. + # + # A file that holds nothing has nothing for this call to destroy, so + # there is no reading to demand: `write` is how an empty file, or one + # whose content is being cleared, is authored. Refusing it would be a + # refusal with no loss behind it — and it printed one, measured + # 2026-10-30: `already holds 0 lines (0 bytes) … none of them has been + # shown to this session`, which asks the caller to read a file with + # nothing in it. + coverage: tuple[int, int] | None = (0, 0) if before.st_size == 0 else None + if coverage is None: + coverage = read_receipts.coverage( + path, + size=before.st_size, + mtime_ns=before.st_mtime_ns, + session=arguments.get("_session_id"), + ) + # ── The numbers are the cheap half of the key; the bytes are the half + # that cannot be preserved by accident ── + # + # A whole-file replacement that keeps the length *and* the + # modification time is ordinary: `cp -p`, `rsync -a`, `tar -x` and + # `git checkout` all put the old timestamp back onto new bytes. A + # receipt keyed on the two numbers alone answered that state as though + # nothing had moved — measured 2026-10-30: `'AAAAAAAAAA'` read, + # `'BBBBBBBBBB'` written back at the same length and same `mtime_ns`, + # whole-file write answered, and content this session never saw was + # destroyed. So a receipt that claims completeness is only believed + # once the bytes agree with the ones it was filed on, and a subject + # whose bytes cannot be read is refused rather than assumed unchanged. + # The bytes are asked about only where a loss is possible: a subject + # holding nothing cannot be destroyed, and the synthetic `(0, 0)` above + # is completeness with no receipt behind it — asking it about bytes would + # turn the empty-file carve-out back into the refusal it replaced. + if before.st_size and coverage is not None and coverage[0] >= coverage[1]: + if not read_receipts.content_matches( + path, + size=before.st_size, + mtime_ns=before.st_mtime_ns, + session=arguments.get("_session_id"), + ): + coverage = None + if coverage is None or coverage[0] < coverage[1]: + return ToolResult( + name="write", + content=_blind_overwrite_refusal(path, before.st_size, coverage), + error=True, + ) try: path.write_text(content, encoding="utf-8") except OSError as e: return ToolResult(name="write", content=f"Error writing file: {e}", error=True) + # The caller has now authored every byte of the file, so a second write in + # the same turn needs no further reading — and the receipt must describe the + # version this call produced, not the one it replaced. + try: + after = path.stat() + except OSError: + after = None + if after is not None: + written_lines = len(content.split("\n")) + if written_lines and content.endswith("\n"): + written_lines -= 1 + read_receipts.record( + path, + size=after.st_size, + mtime_ns=after.st_mtime_ns, + start_line=1, + end_line=written_lines, + total_lines=written_lines, + session=arguments.get("_session_id"), + # The caller authored every byte, so the receipt can name them. + digest=read_receipts.digest_of(path), + ) + action = "Updated" if existed else "Created" logger.debug("write: %s %s (%d chars)", action, path, len(content)) return ToolResult( diff --git a/tests/test_pwsh_tool_v2.py b/tests/test_pwsh_tool_v2.py index 9beae53d..c646c8d1 100644 --- a/tests/test_pwsh_tool_v2.py +++ b/tests/test_pwsh_tool_v2.py @@ -483,15 +483,43 @@ def test_every_mounted_dialect_receives_the_session_cwd(tmp_path): def test_a_dialect_that_is_not_mounted_receives_nothing(tmp_path): - """The set is a set, not a wildcard: an unknown tool is left alone.""" + """The set is a set, not a wildcard: an unknown tool is left alone. + + The name here is one no roster holds. It used to be ``read``, back when no + rule named a non-shell tool — and `read` is named by one now: the caller's + identity rides every read too, because a read that showed a session a file is + what licenses a later `write` over it (issue #2652), and a receipt filed under + a session nobody named is a receipt the write can never find. + """ server = _instantiate() args: dict = {} server._inject_tool_arguments( - "read", args, SimpleNamespace(cwd=tmp_path), SimpleNamespace(sandbox="workspace-write") + "not_a_mounted_tool", + args, + SimpleNamespace(cwd=tmp_path, session_id="s1"), + SimpleNamespace(sandbox="workspace-write"), ) assert args == {} +def test_a_read_is_told_who_is_asking(tmp_path): + """One key for a read, and it is the identity rather than a boundary. + + A read consumes no root and runs nothing, so the tier and the workspace are + none of its business; what it needs is the name to file its reading under, or + the guard that licenses a whole-file replacement cannot be satisfied at all. + """ + server = _instantiate() + args: dict = {} + server._inject_tool_arguments( + "read", + args, + SimpleNamespace(cwd=tmp_path, session_id="s1"), + SimpleNamespace(sandbox="workspace-write"), + ) + assert args == {"_session_id": "s1"} + + # ── the prompt: it names the tool that is registered ───────────────────── diff --git a/tests/test_session_sandbox.py b/tests/test_session_sandbox.py index 17471d49..a14e2f90 100644 --- a/tests/test_session_sandbox.py +++ b/tests/test_session_sandbox.py @@ -226,6 +226,8 @@ def test_the_injection_documents_every_argument_it_writes(): "_parent_cwd", # must not choose, all injected unconditionally "_parent_tools", "_parent_sandbox", + "_session_id", # who is asking: `write` refuses to replace content + # this session was not shown (issue #2652) } assert written == pinned, ( f"the injection writes {sorted(written)}, the guard expects " diff --git a/tests/test_shell_tool_mount.py b/tests/test_shell_tool_mount.py index f75af276..2a2746ba 100644 --- a/tests/test_shell_tool_mount.py +++ b/tests/test_shell_tool_mount.py @@ -159,12 +159,15 @@ def injected(tmp_path): and ``sandbox_roots``, that same fact as paths; rant 2026-10-09T09:43:39), and a real ``Session`` would create directories on disk to prove nothing extra. A stub is kept here rather than a real session because the subject is the - injection rule; empty grants is the "nobody named one" case. + injection rule; empty grants is the "nobody named one" case. ``session_id`` is + the fourth field the rule reads: three tools are told who is asking (issue + #2652), and a stub without one is a session that cannot exist. """ def call(tool: str, args: dict, *, sandbox: str | None = None, cwd: Path | None = None): session = SimpleNamespace( cwd=cwd or tmp_path, sandbox_grants=[], sandbox_roots=[], + session_id="stub-session", ) EmrgServer._inject_tool_arguments(tool, args, session, TaskRequest(sandbox=sandbox)) return args @@ -345,7 +348,21 @@ def test_the_frozen_executor_is_gone(): def test_an_unknown_tool_receives_nothing(injected): - assert injected("read", {"path": "x"}) == {"path": "x"} + assert injected("not_a_mounted_tool", {"path": "x"}) == {"path": "x"} + + +def test_a_read_is_told_only_who_is_asking(injected): + """`read` is named by one rule now, and it is not a boundary. + + This used to be folded into the unknown-tool case above, which read `read` + receives nothing — true while no rule named a non-shell tool. One does now + (issue #2652): the identity a reading is filed under, without which a later + whole-file `write` can never be licensed by what that read showed. What a + read must still not receive is a root or a tier: it runs nothing and writes + nothing, so neither is its business. + """ + args = injected("read", {"path": "x"}) + assert args == {"path": "x", "_session_id": "stub-session"} def test_the_injection_runs_before_every_execution(injected): diff --git a/tests/test_write_needs_a_read.py b/tests/test_write_needs_a_read.py new file mode 100644 index 00000000..9f313937 --- /dev/null +++ b/tests/test_write_needs_a_read.py @@ -0,0 +1,292 @@ +"""`write` replaces what it has been shown, and refuses what it has not. + +The contract these tests pin (issue #2652): a call that would replace the whole +content of an existing file is answered only when this session has been shown +every line of the version that is on disk now. A partial reading, a reading of a +different version, and no reading at all are three states, and only a complete +one permits the replacement. + +Measured cost of not asking (the issue's reproduction): a 5,000-line file read +through the read tool's default 1,000-line cap, written back with what that read +returned, came out at 1,002 lines — and the call reported `Updated`. Three +thousand nine hundred and ninety-eight lines went, silently, in a call that said +it had succeeded. +""" + +import asyncio + +import pytest + +from emrg.tools import read_receipts +from emrg.tools.edit_tool import EditTool +from emrg.tools.read_tool import DEFAULT_MAX_LINES, ReadTool +from emrg.tools.write_tool import WriteTool + + +@pytest.fixture(autouse=True) +def _fresh_receipts(): + """No receipt outlives its test. + + The store is module state keyed by `(session, path)`, and a test that reused + a temporary path would otherwise be answered by the previous test's reading — + a green that measures the fixture rather than the guard. + """ + read_receipts.clear() + yield + read_receipts.clear() + + +def _run(coro): + return asyncio.run(coro) + + +def _read(path, **extra): + return _run(ReadTool().execute({"file_path": str(path), "intent": "read", **extra})) + + +def _write(path, content, **extra): + return _run(WriteTool().execute( + {"file_path": str(path), "content": content, "intent": "write", **extra} + )) + + +def test_blind_overwrite_is_refused_and_the_file_survives(tmp_path): + """The control arm of the defect: nothing read, so nothing may be replaced.""" + target = tmp_path / "existing.txt" + target.write_text("old content", encoding="utf-8") + + result = _write(target, "new content") + + assert result.error + assert target.read_text(encoding="utf-8") == "old content" + # "Read it first" is not actionable on its own: the caller needs to know what + # is at stake, which is the size of what it would have destroyed. + assert "none of them has been shown to this session" in result.content + + +def test_a_capped_read_does_not_license_a_whole_overwrite(tmp_path): + """The reproduction itself: a truncated read, written back, loses the tail.""" + target = tmp_path / "long.md" + original = "".join(f"line {i}: content\n" for i in range(1, 3001)) + target.write_text(original, encoding="utf-8") + + read = _read(target) + assert not read.error + assert "truncated at start_line=" in read.content + + result = _write(target, read.content) + + assert result.error, "a write built from a truncated read must not be answered" + assert target.read_text(encoding="utf-8") == original + # Both numbers, because the gap between them is the caller's next decision. + assert f"{DEFAULT_MAX_LINES:,} of those 3,000 lines" in result.content + assert "3,000 lines" in result.content + + +def test_a_complete_read_licenses_the_overwrite(tmp_path): + """The other direction: the same call is answered once the file has been shown.""" + target = tmp_path / "small.md" + target.write_text("a\nb\nc\n", encoding="utf-8") + + assert not _read(target).error + result = _write(target, "z\n") + + assert not result.error + assert "Updated" in result.content + assert target.read_text(encoding="utf-8") == "z\n" + + +def test_reads_from_where_the_last_one_stopped_accumulate(tmp_path): + """A file longer than one read is covered by reading it in sequence. + + Reaching the end by repeated continuation is the remedy the refusal names, so + it has to be the remedy that works: a guard whose advice does not open the + door would leave every long file unwritable. + """ + target = tmp_path / "long.md" + total = DEFAULT_MAX_LINES + 500 + target.write_text("".join(f"line {i}\n" for i in range(1, total + 1)), encoding="utf-8") + + assert not _read(target).error # lines 1..1000 + assert not _read(target, start_line=DEFAULT_MAX_LINES + 1).error # the rest + + result = _write(target, "replaced\n") + + assert not result.error, result.content + assert target.read_text(encoding="utf-8") == "replaced\n" + + +def test_a_window_into_the_middle_is_not_a_reading_of_the_file(tmp_path): + """Coverage is a prefix, never a count. + + Ten lines seen out of a hundred is not a tenth of the way to a licence: the + lines that were skipped are exactly the ones a whole-file write destroys, and + they are the ones nobody has been shown. + """ + target = tmp_path / "hundred.md" + original = "".join(f"line {i}\n" for i in range(1, 101)) + target.write_text(original, encoding="utf-8") + + assert not _read(target, start_line=50).error + + result = _write(target, "replaced\n") + + assert result.error + assert target.read_text(encoding="utf-8") == original + + +def test_a_file_that_changed_after_the_read_is_read_again(tmp_path): + """A receipt describes a version, and a different version is not covered. + + Without this the guard could be satisfied by reading a file and then having + something else rewrite it — the caller's reading would license destroying + content it never saw. + """ + target = tmp_path / "moving.txt" + target.write_text("first version\n", encoding="utf-8") + assert not _read(target).error + + target.write_text("second version, longer than the first one\n", encoding="utf-8") + + result = _write(target, "replaced\n") + + assert result.error + assert target.read_text(encoding="utf-8").startswith("second version") + + +def test_a_complete_read_from_elsewhere_does_not_license_this_session(tmp_path): + """The receipt belongs to the session that was shown the file. + + `_session_id` is injected by the daemon rather than supplied by the caller for + this reason: the model is the party the guard is about, so a name it chose + would let it read under one identity and write under another. + """ + target = tmp_path / "shared.txt" + target.write_text("content only one session has seen\n", encoding="utf-8") + + assert not _read(target, _session_id="reader").error + + result = _write(target, "replaced\n", _session_id="writer") + + assert result.error + assert target.read_text(encoding="utf-8") == "content only one session has seen\n" + + +def test_what_this_session_wrote_it_may_write_again(tmp_path): + """A file the caller authored needs no reading: it has seen every byte. + + Without this, an agent could not correct its own output within a turn — and + writing a new file, which is the commonest call this tool makes, would be + followed by a refusal against itself. + """ + target = tmp_path / "mine.txt" + + assert not _write(target, "first draft\n").error + result = _write(target, "second draft\n") + + assert not result.error, result.content + assert target.read_text(encoding="utf-8") == "second draft\n" + + +def test_an_empty_existing_file_needs_no_reading(tmp_path): + """Nothing to be shown is nothing to lose. + + A zero-line file has no content for a write to destroy, so refusing here + would be a refusal with no harm behind it — the guard is about the loss, not + about the act of opening an existing path. + """ + target = tmp_path / "empty.txt" + target.write_text("", encoding="utf-8") + + result = _write(target, "content\n") + + assert not result.error, result.content + assert target.read_text(encoding="utf-8") == "content\n" + + +# ── The receipt is the bytes, not just the numbers ──────────────────────── +# +# Review of this branch found two states the first version could not tell apart, +# and both are the harm direction: one let a write through, the other refused a +# write that had no unseen content behind it. + + +def test_a_same_length_same_mtime_change_is_not_the_version_that_was_read(tmp_path): + """Size and `mtime_ns` agreeing is not the file agreeing. + + A replacement that keeps the length *and* the modification time is ordinary + rather than exotic — `cp -p`, `rsync -a`, `tar -x` and `git checkout` all put + the old timestamp back onto new bytes — and a receipt keyed on those two + numbers alone answers it as though nothing moved. Measured on the first + version of this guard: `'AAAAAAAAAA'` read, `'BBBBBBBBBB'` put in its place at + the same length and the same `mtime_ns`, whole-file write answered, and + content the session never saw destroyed. + """ + import os + + target = tmp_path / "same.txt" + target.write_text("AAAAAAAAAA\n", encoding="utf-8") + filed = target.stat() + + assert not _read(target).error + + target.write_text("BBBBBBBBBB\n", encoding="utf-8") + os.utime(target, ns=(filed.st_atime_ns, filed.st_mtime_ns)) + now = target.stat() + assert (now.st_size, now.st_mtime_ns) == (filed.st_size, filed.st_mtime_ns), ( + "the fixture must reach the state under test: identical size and mtime_ns" + ) + + result = _write(target, "CCCCCCCCCC\n") + + assert result.error, "a write over bytes this session never saw must be refused" + assert target.read_text(encoding="utf-8") == "BBBBBBBBBB\n" + + +def test_a_file_this_session_wrote_and_then_edited_stays_writable(tmp_path): + """`edit` is a writer too, and it must not leave a stale receipt behind. + + Every byte of the new version is known to a session that authored the text and + then replaced a string it named, so the loop write → edit → write has to stay + open; before this rule it was refused with "none of them has been shown to this + session" about text the session had just written. + """ + target = tmp_path / "edited.txt" + + assert not _write(target, "alpha\nbeta\n", _session_id="s1").error + edit = _run(EditTool().execute({ + "file_path": str(target), "old_string": "beta", "new_string": "gamma", + "intent": "edit", "_session_id": "s1", + })) + assert not edit.error, edit.content + assert target.read_text(encoding="utf-8") == "alpha\ngamma\n" + + result = _write(target, "alpha\ngamma\ndelta\n", _session_id="s1") + + assert not result.error, result.content + assert target.read_text(encoding="utf-8") == "alpha\ngamma\ndelta\n" + + +def test_an_edit_does_not_license_a_rewrite_of_a_file_read_only_in_part(tmp_path): + """The other direction: the receipt `edit` files is only for a complete reading. + + A session shown the first 1,000 lines of a longer file has not seen the rest, + and an edit to a line it did see does not change that — so the whole-file + rewrite stays refused, which is the direction that cannot destroy anything. + """ + target = tmp_path / "long.txt" + total = DEFAULT_MAX_LINES + 50 + original = "".join(f"line {i}\n" for i in range(1, total + 1)) + target.write_text(original, encoding="utf-8") + + assert not _read(target).error # lines 1..1000 only + edit = _run(EditTool().execute({ + "file_path": str(target), "old_string": "line 5\n", "new_string": "line five\n", + "intent": "edit", "_session_id": "s1", + })) + assert not edit.error, edit.content + + result = _write(target, "replaced\n", _session_id="s1") + + assert result.error + assert target.read_text(encoding="utf-8").startswith("line 1\nline 2\n") diff --git a/tests/test_write_tool.py b/tests/test_write_tool.py index 9efca643..b505d8a5 100644 --- a/tests/test_write_tool.py +++ b/tests/test_write_tool.py @@ -7,6 +7,7 @@ import pytest +from emrg.tools.read_tool import ReadTool from emrg.tools.write_tool import WriteTool @@ -37,6 +38,12 @@ def test_write_overwrites_file(temp_dir): filepath = temp_dir / "existing.txt" filepath.write_text("old content") + # An existing file is replaced only once this session has been shown it + # (issue #2652): the overwrite below is the one that is *allowed*, and + # `tests/test_write_needs_a_read.py` holds the refusal it is the control for. + read = _run(ReadTool().execute({"file_path": str(filepath), "intent": "read"})) + assert not read.error + result = _run(tool.execute({ "file_path": str(filepath), "content": "new content",