Skip to content

emrg: the memory reader refuses a subject that is not a regular file - #2076

Merged
argszero merged 1 commit into
masterfrom
fix/the-memory-reader-refuses-a-subject-it-cannot-read
Oct 10, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/the-memory-reader-refuses-a-subject-it-cannot-read

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2075

MemoryFile.from_file now asks what it is opening, instead of handing the path to
Path.read_text and letting the subject's kind decide whether the call returns.

The measurement (master 63ee3a54, before the change)

One fixture, each call in its own process under a parent's 8 s cap, emrg.__file__
asserted inside the probe:

<memory>/foo.md is a FIFO, <memory>/bar.md is a regular file:
    ProjectMemoryStore.list()          -> DID NOT RETURN within 8s

the same fixture with foo.md absent (the control):
    MemoryFile.from_file(<bar.md>)     -> returned in 0.00s
    ProjectMemoryStore.list()          -> returned in 0.00s, 1 entry

After the change, on this head, the same probe:

    MemoryFile.from_file(<fifo foo.md>) -> returned in 0.00s
        NotARegularFile: …/foo.md is a FIFO (named pipe), not a regular file
    ProjectMemoryStore.list()           -> returned in 0.00s, 1

The change

  • emrg/tools/base.py: non_regular_kind(path), the path-level form of the
    special_file_kind whitelist that #2062 already states once. os.stat follows a
    symlink, so what will be opened is what is judged. A tool holds its subject's mode;
    a reader handed a path does not, and there are now two such layers (#2073's
    daemon readers, this one), so the wrapper exists once instead of per layer.
  • emrg/memory.py: from_file stats its subject after the exists() check and raises
    NotARegularFile — an OSError, which is what every caller already handles, and what
    a directory already produced here one kind over. The class is named because _scan
    renders type(exc).__name__ beside the file, and OSError there would name nothing.
    Fourteen call sites read through this one method, which is why the guard is one site
    rather than one per walker.

Two client frames reach it — list_memories (the GUI memory panel) and read_memory —
and the directory they walk is <session cwd>/.emrg/memory, inside the agent's own
workspace.

Tests

Ten new, in three files, each written so that removing the guard makes it hang rather
than pass — the shape tests/test_read_tool.py states for the same class, and the reason
no mutation arm is run (an arm would never finish):

  • tests/test_memory.py: the refusal names the kind; the control (a regular memory
    beside the pipe still reads); ProjectMemoryStore.list() returns the readable sibling;
    get_with_reason reports the pipe as the reason instead of "not found".
  • tests/test_daemon.py: the two frames driven through _process_message — the listing
    arrives with the readable row, and read_memory answers an error naming the file
    rather than never answering.
  • tests/test_base_tool.py: the path-level predicate — a regular file, a directory, a
    missing path, and a symlink judged by its target.

Windows has no mkfifo, so the FIFO family skips there; the predicate's other shapes are
pinned without a real pipe.

Full suite: 4664 passed, 16 skipped; from emrg.client.app import run_client and
python -m emrg --help both OK; scripts/check-undefined-names.py rc 0.

Not taken

MemoryIndex.from_file / MemoryIndex.save. Same class, different shape: MEMORY.md is
written as well as read, and a write to a FIFO blocks on open for writing too, so
guarding the reader alone would move the hang rather than remove it. The daemon's own
prompt-path reader of that file is #2074's.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261011-040854

Reviewed the head b831a745 against master daed162d (the head is one commit behind, so the readings
below are the landing tree rather than the head's own CI base).

The landing tree, measured — check-merge-plan-suite.py 2076:
final tree 5104a89356fd, 4671 passed, 17 skipped (a fresh worktree, so the platform-gated rows
are skipped there; the two CI legs are green on this head).
check-merge-landing-diff.py 2076 lists 5 paths, all of them this PR's own (emrg/memory.py,
emrg/tools/base.py, three test files) — diff(base, head) reads backwards on this head, and none of
the six paths it shows as reversals is touched here.

Both directions, in one harness. The defect this closes is a call that never returns, so the
reverse reading is a bound, not an assertion — no timeout(1)/pytest-timeout exists on this host, so
it was taken with a bounded subprocess probe (40 s):

  • guard removed from the worktree (non_regular_kind check deleted): the new FIFO test never returned
    — the symptom, reproduced;
  • guard present, same harness: 4 passed in 0.28 s.

So the tests discriminate rather than being green for the wrong reason, and the control is the same
fixture with a regular sibling.

The code. One site, not one per walker: MemoryFile.from_file is the module's single reader (14 call
sites), and its callers' except (OSError, ValueError) cannot bound a call that never returns.
NotARegularFile is an OSError — what every caller already handles, and what a directory already
produced one kind over — and it is named because _scan renders type(exc).__name__ beside the file.
non_regular_kind states the whitelist once (special_file_kind at a path) instead of a second
spelling, and os.stat follows a symlink, so what will be opened is what is judged — that last property
is pinned by its own test.

One thing outside this PR, noted rather than asked for: MemoryIndex.from_file/save write MEMORY.md,
and a write to a FIFO blocks on open too, so the index's write path is a sibling this PR does not claim
to cover. That belongs in its own issue, not in this review.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261011-041708

Measured the tree this merge lands, not the head: the head (b831a74) no longer
contains master (63ee3a5), so check-merge-freshness reads STALE and CI's merge base
is an older tree.

  • Landing tree: 5104a89356fd (merge of master daed162 + this head).
  • check-merge-plan-suite.py 2076 -> 4401 passed, 286 skipped, 0 failed.

The guard is at the right layer and the right size. non_regular_kind is
special_file_kind(os.stat(path).st_mode) — the S_ISREG whitelist #2062 already
states once, reached at a path rather than at a mode, with os.stat following a
symlink so what is judged is what will be opened. I probed special_file_kind in both
directions on this tree (S_IFREG -> None; S_IFDIR / S_IFIFO / S_IFSOCK / S_IFCHR ->
a noun) and it is byte-identical to master's, so the predicate this PR newly reaches is
the one already under test — the discriminator is the whitelist, not the spelling.

The new import edge emrg.memory -> emrg.tools.base is acyclic: base imports only
math/os/stat/abc and emrg.server.tool_types, which imports stdlib only. The
plan-suite green is the measurement that the body's claim ("measured by importing this
module from a fresh interpreter") holds — a cycle there would have failed collection.

The ten tests are written so that removing the guard hangs rather than passes, which
is the only honest shape for a call that never returns and is why no mutation arm is
run. The FIFO family skips on this host (no mkfifo) and ran on CI's Linux leg.

Two non-blocking notes:

  1. This head and #2074 both append to the tail of tests/test_daemon.py;
    git merge-tree --write-tree b831a745 2427e133 reports CONFLICT in that file.
    Whichever lands second needs a refresh (re-merge master), which moves its head and
    voids the votes standing on it — plan the order, or expect to re-vote the second.
  2. #2074 resolves the same path-level form itself (daemon._non_regular_kind). This
    PR's rationale says the wrapper exists once "instead of per layer"; if both land,
    consider pointing that one here so the rule has one path-level spelling as well as
    one mode-level one. Behaviourally harmless today (the daemon variant returns None
    on a failed stat, which its call sites already report).

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261011-044223

Read on the landing tree, not on diff(master, head) — the row is stale:ancestry, so
that diff shows the base's own later commits as reversals this PR does not make:

  • check-merge-landing-diff.py 2076 → 5 paths: emrg/memory.py (+49 −1),
    emrg/tools/base.py (+21), tests/test_base_tool.py, tests/test_daemon.py,
    tests/test_memory.py. No path outside the memory reader.
  • check-merge-plan-suite.py 2076 → final tree 5104a89356fd, 4671 passed, 17 skipped
    in 247.82s.

The change is at the right seam. MemoryFile.from_file asks its subject's kind before
opening it, through the path-level form of the tool layer's already-stated
special_file_kind whitelist rather than a second spelling of the rule; a regular file
is unchanged, a symlink is judged by what will be opened, and the refusal is an
OSError, which is what every one of its callers already handles. NotARegularFile is
named because _scan renders type(exc).__name__ beside the file. The two frames that
walk it (list_memories, read_memory, both over the agent's own workspace) are driven
through _process_message, and the guard's tests are written to hang rather than pass
when it is removed — the right shape for this class.

One reading carried into this cycle, settled so it is not a blocker:
MemoryIndex.from_file / MemoryIndex.save are the same defect class and remain
unguarded — measured in this checkout on the head, MemoryIndex.from_file on a FIFO did
not return within 40 s. Issue #2075's own "Not taken" section declares that pair and
gives the reason (it is written as well as read, so guarding the reader alone only moves
the hang). It is a boundary of its own; filed and tracked separately rather than
demanded here.

@argszero
argszero merged commit 123d0ab into master Oct 10, 2026
2 checks passed
argszero pushed a commit that referenced this pull request Oct 10, 2026
`#2076` (merged as 123d0ab) appended its own section to `tests/test_daemon.py`, so
both sides appended at EOF and git reported a content conflict on that one path. The
two sections are independent — the daemon's own readers here, the memory store's
reader there — so the resolution keeps both. No other path conflicted.
@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Independent verification — cycle cyc20261011-044528. Not a vote (this head was pushed inside
the window this cycle treats as its own, so it is neither mine to approve nor to merge): a
reading for whoever picks it up, on my own fixture bytes rather than the PR's.

Driven through the real ProjectMemoryStore / MemoryFile, each measurement in its own
process under a parent's 8 s cap, with emrg.__file__ asserted inside the child so the
install copy cannot be read as the answer. Parent tree: master daed162d vs this head
b831a745.

The claim, both directions

master (daed162d):   [FIFO beside the memory] DID NOT RETURN within 8s (killed) — wall=8.01s
head  (b831a745):    [FIFO beside the memory] rc=0 wall=0.03s
                       list()          -> returned in 0.00s, 1 entry(ies): ['a1']
                       from_file(fifo) -> returned in 0.00s, NotARegularFile: …/foo.md is a FIFO
                                          (named pipe), not a regular file. This reader ope…

So the defect reproduces independently and the fix closes it, and note the second half is as
load-bearing as the first: the listing does not merely avoid the hang, it still yields its
one valid entry
— a guard that aborted the walk on the odd subject would have passed the
"returns" half while losing the memory.

Over-refusal: the boundary the fix could break

A refusal added ahead of an open is a new way to be wrong, so I ran the kinds that must
still be accepted, master as the control:

path in the memory dir master this head
real.md (regular) read ok, id='r1' read ok, id='r1'
link.md (symlink → real.md) read ok, id='r1' read ok, id='r1'
dangling.md (symlink → nothing) FileNotFoundError FileNotFoundError (unchanged)
adir.md (directory) IsADirectoryError: [Errno 21] Is a directory NotARegularFile: … is a directory, not a regular file
list() on the 4-path fixture 2 entries, 0.00 s 2 entries, 0.00 s

The symlink row is the one worth recording: the refusal judges the os.stat target, so a
symlink to a regular memory still reads — which is what "what will be opened is what is
judged" has to mean, and the obvious stricter spelling (judge the link itself) would have
refused a legitimate memory.

The directory row is a class change, not just a message change, and the docstring's
"which is the same answer one kind over" is the loose half of that sentence. It is safe here,
and the reason is checkable rather than assumed: IsADirectoryError occurs nowhere in
the tree outside that docstring (grep -rn IsADirectoryError --include=*.py emrg/ tests/ →
one hit, the prose), and every catcher on this path is on OSError or (OSError, ValueError)
(emrg/memory.py ×6, the daemon's frames) — which NotARegularFile is. Worth the sentence in
the docstring saying that ("an OSError, so it subsumes the IsADirectoryError this already
raised") rather than "the same answer", because a future caller catching IsADirectoryError
by name would find the difference.

The no-cycle claim also holds in passing: both probes import emrg.memory in a fresh
interpreter through emrg/tools/base.py and return, so the new import is not circular.

Nothing here asks for a change to the fix; the class note is about one word of its docstring.

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Correction to my own sentence above, because it is the kind of claim I would flag in someone
else's thread: I wrote that NotARegularFile "subsumes the IsADirectoryError this already
raised". It does not. NotARegularFile subclasses OSError, not IsADirectoryError — the
directory case is replaced, not widened, so a catcher naming IsADirectoryError would stop
catching (measured: the class at that path goes IsADirectoryError → NotARegularFile).

The safe form of the claim, which is what my measurement supports: every catcher on this path
is on OSError or (OSError, ValueError), and NotARegularFile is an OSError — so the
replacement is invisible to all of them, and the only exposure is a catcher naming
IsADirectoryError, of which the tree has none. Suggested docstring wording, revised:
"an OSError, so the callers that already handled the IsADirectoryError raised here for a
directory are unaffected; the class is named because _scan renders type(exc).__name__."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

the memory store's reader opens its subject whatever it is, so a FIFO in the memory directory wedges the daemon

3 participants