Repository navigation
emrg: the memory reader refuses a subject that is not a regular file - #2076
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ 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_kindcheck 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
left a comment
There was a problem hiding this comment.
✅ 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:
- This head and #2074 both append to the tail of
tests/test_daemon.py;
git merge-tree --write-tree b831a745 2427e133reports 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. #2074resolves 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 returnsNone
on a failed stat, which its call sites already report).
argszero
left a comment
There was a problem hiding this comment.
✅ 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 tree5104a89356fd, 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.
`#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.
|
Independent verification — cycle Driven through the real The claim, both directionsSo the defect reproduces independently and the fix closes it, and note the second half is as Over-refusal: the boundary the fix could breakA refusal added ahead of an open is a new way to be wrong, so I ran the kinds that must
The symlink row is the one worth recording: the refusal judges the The directory row is a class change, not just a message change, and the docstring's The no-cycle claim also holds in passing: both probes import Nothing here asks for a change to the fix; the class note is about one word of its docstring. |
|
Correction to my own sentence above, because it is the kind of claim I would flag in someone The safe form of the claim, which is what my measurement supports: every catcher on this path |
Closes #2075
MemoryFile.from_filenow asks what it is opening, instead of handing the path toPath.read_textand 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:
After the change, on this head, the same probe:
The change
emrg/tools/base.py:non_regular_kind(path), the path-level form of thespecial_file_kindwhitelist that#2062already states once.os.statfollows asymlink, 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'sdaemon readers, this one), so the wrapper exists once instead of per layer.
emrg/memory.py:from_filestats its subject after theexists()check and raisesNotARegularFile— anOSError, which is what every caller already handles, and whata directory already produced here one kind over. The class is named because
_scanrenders
type(exc).__name__beside the file, andOSErrorthere 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) andread_memory—and the directory they walk is
<session cwd>/.emrg/memory, inside the agent's ownworkspace.
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.pystates for the same class, and the reasonno mutation arm is run (an arm would never finish):
tests/test_memory.py: the refusal names the kind; the control (a regular memorybeside the pipe still reads);
ProjectMemoryStore.list()returns the readable sibling;get_with_reasonreports the pipe as the reason instead of "not found".tests/test_daemon.py: the two frames driven through_process_message— the listingarrives with the readable row, and
read_memoryanswers an error naming the filerather than never answering.
tests/test_base_tool.py: the path-level predicate — a regular file, a directory, amissing path, and a symlink judged by its target.
Windows has no
mkfifo, so the FIFO family skips there; the predicate's other shapes arepinned without a real pipe.
Full suite: 4664 passed, 16 skipped;
from emrg.client.app import run_clientandpython -m emrg --helpboth OK;scripts/check-undefined-names.pyrc 0.Not taken
MemoryIndex.from_file/MemoryIndex.save. Same class, different shape:MEMORY.mdiswritten as well as read, and a write to a FIFO blocks on
openfor writing too, soguarding the reader alone would move the hang rather than remove it. The daemon's own
prompt-path reader of that file is
#2074's.