From b831a745a2fbac455b86593075a212bd6ad83959 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Sun, 11 Oct 2026 03:30:43 +0800 Subject: [PATCH] emrg: the memory reader refuses a subject that is not a regular file --- emrg/memory.py | 50 +++++++++++++++++++++- emrg/tools/base.py | 21 +++++++++ tests/test_base_tool.py | 45 +++++++++++++++++++- tests/test_daemon.py | 57 +++++++++++++++++++++++++ tests/test_memory.py | 94 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 265 insertions(+), 2 deletions(-) diff --git a/emrg/memory.py b/emrg/memory.py index da14705f..61ce6521 100644 --- a/emrg/memory.py +++ b/emrg/memory.py @@ -30,6 +30,13 @@ from pathlib import Path from typing import ClassVar, Optional +# The "is this path a regular file" rule is the tool layer's (`special_file_kind`, a +# whitelist of `S_ISREG`), reached at a path rather than at a mode. Imported rather than +# restated: the boundary is one fact, and a second spelling of it is a second boundary +# that can drift. `emrg.tools.base` imports only `emrg.server.tool_types`, so this adds +# no cycle — measured by importing this module from a fresh interpreter. +from emrg.tools.base import non_regular_kind + logger = logging.getLogger(__name__) # ── Helpers ──────────────────────────────────────────────────────── @@ -242,6 +249,24 @@ def is_index_row(line: str) -> bool: # accepts any `## ` heading; this only decides where new rows file. INDEX_TYPE_ORDER = ("user", "feedback", "project", "reference", "decision", "task") + +class NotARegularFile(OSError): + """A memory path whose kind rules out reading it, raised instead of opened. + + An **open is not a read**: on a FIFO it blocks until a writer appears, on a socket + until a connection does, and a device never ends. ``from_file`` opened its subject + without asking, and its callers' ``except (OSError, ValueError)`` cannot bound the + consequence — nothing is raised, the call simply never returns, and it runs inside a + coroutine the daemon awaits on its event loop, so the block is the whole daemon. + + An ``OSError`` on purpose: that is what every caller of ``from_file`` already handles, + and it is what a *directory* already produced here (``IsADirectoryError``), which is + the same answer one kind over. Naming the kind in the class rather than only in the + message matters because `_scan` renders ``type(exc).__name__`` beside the file, and + ``OSError`` there would name nothing. + """ + + # ── MemoryFile ───────────────────────────────────────────────────── @@ -300,9 +325,32 @@ def filename(self) -> str: @classmethod def from_file(cls, path: Path) -> MemoryFile: - """Parse a memory file from disk.""" + """Parse a memory file from disk. + + The subject's **kind** is asked before it is opened, because opening it is what + decides whether this call returns at all (`NotARegularFile`). Measured + 2026-10-11 on master `63ee3a54`, one fixture (a named pipe beside a regular + memory), each call in its own process under a parent's 8 s cap: + ``ProjectMemoryStore.list()`` **did not return**, while the same fixture without + the pipe returned in 0.00 s. Two client frames walk this reader — `list_memories` + and `read_memory` — and the directory is the agent's own workspace, so anything + running there can put a pipe in it. + + Every reader in this module comes through here, which is why the guard is one + site rather than one per walker: `_scan`, `list`, `_rebuild_index`, `update`, + `merge`, `promote_to_project`, `get_by_filename`, `_resolve_filename`. + """ if not path.exists(): raise FileNotFoundError(f"Memory file not found: {path}") + kind = non_regular_kind(path) + if kind: + raise NotARegularFile( + f"{path} is {kind}, not a regular file. This reader opens its subject " + f"directly, and opening this one blocks until a peer appears — a FIFO " + f"waits for a writer, a socket for a connection, a device never ends — " + f"so the call would never return, and neither would the daemon, which " + f"runs its readers on its event loop." + ) content = path.read_text(encoding="utf-8") return cls.from_text(content, _filename=path.name) diff --git a/emrg/tools/base.py b/emrg/tools/base.py index ed8342ac..d5205edd 100644 --- a/emrg/tools/base.py +++ b/emrg/tools/base.py @@ -7,6 +7,7 @@ from __future__ import annotations import math +import os import stat from abc import ABC, abstractmethod @@ -171,6 +172,26 @@ def special_file_kind(mode: int) -> str | None: return "not a regular file" +def non_regular_kind(path: str | os.PathLike[str]) -> str | None: + """Name a path's kind when it is not a regular file — :func:`special_file_kind` at a path. + + ``special_file_kind`` reads a ``st_mode`` its caller already has, which is the shape a + *tool* wants: it stats its own subject to describe it to the caller. A reader that was + handed a **path** has no mode, and two of those exist outside the tool layer — the + daemon's own readers (`#2073`) and the memory store's (`from_file`). Both need the same + whitelist, so it is stated once and reached two ways rather than restated per layer. + + ``os.stat`` follows a symlink, so a link to a pipe is a pipe: what the caller is about + to open is what is judged, not how it was spelled. + + :param path: the path a reader was handed. + :returns: the noun phrase for a non-regular subject, or ``None`` when it is a regular + file. A missing path raises ``FileNotFoundError`` from the ``stat`` — the answer + callers of a missing file already handle. + """ + return special_file_kind(os.stat(path).st_mode) + + def special_file_refusal(path: object, kind: str) -> str: """The one refusal the file tools give a subject that is not a regular file. diff --git a/tests/test_base_tool.py b/tests/test_base_tool.py index a3ac5add..5ea4d6dc 100644 --- a/tests/test_base_tool.py +++ b/tests/test_base_tool.py @@ -1,10 +1,11 @@ """Tests for the tool base class.""" +import os import stat import pytest -from emrg.tools.base import ToolExecutor, special_file_kind +from emrg.tools.base import ToolExecutor, non_regular_kind, special_file_kind def test_tool_executor_is_abstract(): @@ -72,3 +73,45 @@ def test_special_file_kind_ignores_permission_bits(): """Only the type bits decide; a read-only or world-writable regular file is regular.""" for perms in (0o000, 0o400, 0o444, 0o777): assert special_file_kind(stat.S_IFREG | perms) is None + + +# ── `non_regular_kind`: the same rule, at a path ── +# +# A tool holds its subject's mode already; a *reader* handed a path does not. The memory +# store's reader (issue #2075) and the daemon's (`#2073`) are both readers, so the +# whitelist is reached at a path rather than restated per layer. These pin the stat's +# fields of view — what it follows, and what it raises for a path that is not there. + + +def test_non_regular_kind_is_none_for_a_regular_file(tmp_path): + """The ordinary subject: the caller opens it, and `None` is the licence to.""" + regular = tmp_path / "notes.md" + regular.write_text("hello\n", encoding="utf-8") + assert non_regular_kind(regular) is None + + +def test_non_regular_kind_names_a_directory(tmp_path): + """A mode built from `S_IF*` is not needed here — a real directory carries one.""" + assert non_regular_kind(tmp_path) == "a directory" + + +def test_non_regular_kind_raises_for_a_path_that_is_not_there(tmp_path): + """Missing is the callers' other answer, and it stays an exception rather than a noun.""" + with pytest.raises(FileNotFoundError): + non_regular_kind(tmp_path / "absent.md") + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs") +def test_non_regular_kind_judges_a_symlink_by_what_will_be_opened(tmp_path): + """`os.stat` follows the link, so a link to a pipe is a pipe. + + Stated because the decision is the stat's and not the spelling's: opening the link + opens the pipe, so a predicate that judged the link itself would license the open it + exists to refuse. + """ + fifo = tmp_path / "pipe" + os.mkfifo(fifo) + link = tmp_path / "link.md" + link.symlink_to(fifo) + assert non_regular_kind(link) == "a FIFO (named pipe)" + assert non_regular_kind(fifo) == "a FIFO (named pipe)" diff --git a/tests/test_daemon.py b/tests/test_daemon.py index 941f4cd4..96fb238f 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -4310,3 +4310,60 @@ async def stream(messages, tools=None): "a genuinely completed round no longer stamps the marker the #1114 alarm " "measures — the empty-answer guard must not have swallowed it" ) + + +# ── a memory subject that is not a regular file ──────────────────── +# +# The sibling of the section above, one layer down: `list_memories` and `read_memory` +# walk `/.emrg/memory` through `MemoryStore`, whose single reader used to open +# whatever it was handed. A pipe there is not a slow request — `open` on a FIFO never +# returns, the walk is synchronous inside the handler, and the handler is awaited on +# the event loop, so the frame that never gets its answer is the daemon that never +# serves another (issue #2075). The guard is `emrg.memory.MemoryFile.from_file`. +# +# Driving the dispatch rather than the store is the point: the store tolerates one +# unreadable file by design, and what has to be proved is that the *frame* arrives. + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs") +def test_a_memory_that_is_a_named_pipe_costs_the_row_not_the_listing(tmp_path): + """A pipe in the memory directory is skipped; the readable rows still arrive.""" + server = _make_server() + directory = _memory_project(tmp_path, b"# Memory Index\n") + os.mkfifo(directory / "pipe.md") + (directory / "note.md").write_text( + "---\nid: aaaa1111\ntype: reference\nscope: project\nstatus: active\n" + "title: A note\n---\n\nbody\n", + encoding="utf-8", + ) + + frames = _drive(server, {"type": "list_memories", "scope": "project", "cwd": str(tmp_path)}) + + assert len(frames) == 1, f"expected one frame, got {frames!r}" + frame = frames[0] + assert frame["type"] == "memories_list" + assert [m["id"] for m in frame["memories"]] == ["aaaa1111"], ( + "the pipe is not a memory, and it must not cost the memory beside it" + ) + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs") +def test_a_read_memory_through_a_pipe_directory_answers_rather_than_hangs(tmp_path): + """`read_memory` walks the same directory to match an id; the answer must come back. + + The reason names the file and its kind, because "Memory not found" would be a claim + this walk never measured — the pipe is a file the walk could not finish looking at. + """ + server = _make_server() + directory = _memory_project(tmp_path, b"# Memory Index\n") + os.mkfifo(directory / "pipe.md") + + frames = _drive( + server, + {"type": "read_memory", "scope": "project", "memory_id": "aaaa1111", "cwd": str(tmp_path)}, + ) + + assert len(frames) == 1, f"expected one frame, got {frames!r}" + assert "error" in frames[0], frames[0] + assert "pipe.md" in frames[0]["error"], frames[0]["error"] + assert "NotARegularFile" in frames[0]["error"], frames[0]["error"] diff --git a/tests/test_memory.py b/tests/test_memory.py index f45d5bf1..b35e28ce 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -1,5 +1,6 @@ """Tests for the EMRG memory module.""" +import os import tempfile from pathlib import Path @@ -10,6 +11,7 @@ INDEX_TITLE_MAX_CHARS, MemoryFile, MemoryIndex, + NotARegularFile, ProjectMemoryStore, SessionMemoryStore, generate_id, @@ -769,3 +771,95 @@ def test_the_filename_field_stays_out_of_the_file_format(self, project_store): reloaded = MemoryFile.from_file(project_store.directory / mem.filename) assert reloaded._filename == mem.filename assert reloaded.to_markdown() == text, "a load → save rewrote the file" + + +# ── the subject has to be a regular file ────────────────────────── +# +# An open is not a read: on a FIFO a read blocks until a writer appears, on a socket +# until a connection does, and a device never ends. The three file tools ask +# `special_file_kind` before opening (#2062) and the daemon's own readers took the same +# guard (#2073). This is the third layer, and it is the memory module's single reader: +# fourteen call sites, of which `list_memories` and `read_memory` are the two client +# frames — and the directory they walk is `/.emrg/memory`, inside the +# agent's own workspace, so anything running there can put a pipe in it. +# +# Each test is written so that removing the guard makes it **hang** rather than pass: +# the condition is a call that never returns, and no assertion can bound that (the shape +# `tests/test_read_tool.py` states for the same class). Windows has no `mkfifo`, so the +# family skips there. + + +def _needs_mkfifo(func): + return pytest.mark.skipif( + not hasattr(os, "mkfifo"), reason="this platform has no FIFOs" + )(func) + + +@_needs_mkfifo +def test_the_reader_refuses_a_named_pipe_instead_of_blocking(tmp_path): + """`from_file` names the kind rather than opening the subject.""" + fifo = tmp_path / "notes.md" + os.mkfifo(fifo) + + with pytest.raises(NotARegularFile) as excinfo: + MemoryFile.from_file(fifo) + + assert "a FIFO (named pipe)" in str(excinfo.value), str(excinfo.value) + assert "not a regular file" in str(excinfo.value), str(excinfo.value) + + +@_needs_mkfifo +def test_the_reader_still_reads_a_regular_file_beside_a_pipe(tmp_path): + """The control: the refusal is about the kind, not about the directory holding it.""" + os.mkfifo(tmp_path / "pipe.md") + regular = tmp_path / "notes.md" + regular.write_text( + "---\nid: aaaa1111\ntype: reference\nscope: project\nstatus: active\n" + "title: A note\n---\n\nbody\n", + encoding="utf-8", + ) + + assert MemoryFile.from_file(regular).id == "aaaa1111" + + +@_needs_mkfifo +def test_the_store_lists_the_memories_beside_a_pipe(temp_cwd): + """The measured route: `list_memories` walks every `*.md`, so a pipe in the + directory is a client frame that never gets its answer. + + Asserted through `ProjectMemoryStore.list()` rather than through `from_file`, + because the walk's `except (OSError, ValueError)` is what turns the refusal into a + skipped file — a width the reader alone does not decide. + """ + directory = temp_cwd / ".emrg" / "memory" + directory.mkdir(parents=True) + (directory / "MEMORY.md").write_text("# Memory Index\n", encoding="utf-8") + os.mkfifo(directory / "pipe.md") + store = ProjectMemoryStore(temp_cwd) + store.create("reference", "A note", "body") + + assert [m.title for m in store.list()] == ["A note"], ( + "the pipe is not a memory and the readable sibling still is" + ) + + +@_needs_mkfifo +def test_the_scan_names_the_pipe_as_a_reason_rather_than_never_answering(temp_cwd): + """`read_memory`'s route: `_scan` walks the same directory to match an id. + + An id that is not there must still come back as an answer, and the pipe must be + reported as the reason the walk could not finish — "not found" would be a claim this + walk did not measure. + """ + directory = temp_cwd / ".emrg" / "memory" + directory.mkdir(parents=True) + os.mkfifo(directory / "pipe.md") + store = ProjectMemoryStore(temp_cwd) + + mem, reason = store.get_with_reason("aaaa1111") + + assert mem is None + assert "pipe.md" in reason, reason + assert "NotARegularFile" in reason, ( + "the reason must name the kind's verdict — `OSError` there would name nothing" + )