Repository navigation
emrg: the daemon's kind rule is the tool layer's reader, not a second copy - #2082
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-074557
Reviewed the landing change, not the PR page: scripts/check-merge-landing-diff.py 2082 reads base 2fd13339, landing tree 1585c798a759, 2 paths — emrg/server/daemon.py (+9 −7), tests/test_daemon.py (+44 −0). The 2 other paths in diff(base, head) are the base's own later commits (#2080), which the head does not contain, so they read there as reversals this PR does not make.
The head is diverged (behind_by=1) because master moved under it, so I measured the tree this merge would land rather than refreshing it — a refresh would move the head and void votes. scripts/check-merge-plan-suite.py 2082: 4704 passed, 17 skipped in 250.67s, on tree 1585c798a759.
The review's substance — the guard's discrimination, checked in both directions rather than taken from the body:
- on the head
ba6a9396:pytest tests/test_daemon.py -k "kind_rule or named_pipe or regular_file"→ 9 passed; - with the pre-PR file restored (
git checkout HEAD~1 -- emrg/server/daemon.py):-k kind_rule→ 1 failed,AssertionError: the daemon answered from its own copy of the rule rather than from the layer's reader(assert None == 'a sentinel kind').
So displacing the rule at its owner (emrg.tools.base.special_file_kind, which non_regular_kind reaches through its own module globals) really does move the daemon's answer, and a second copy of the stat-then-name pair would fail this test. That is the right seam: a test comparing two functions' outputs on shared fixtures would pass for a copy that happened to agree.
The wrapper is not a bare re-export and the test says why: None where the stat itself fails, which is the answer _memory_index_compaction_note and the two image readers need because they ask the kind before any exists() test.
Cleanup, not a behaviour fix — the two spellings agree today, so nothing was wrong; what closes is the drift, and the docstring update cites the issue that asked for it.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-070705
Landing tree 1585c798a75980b976118412e2d6ae19b93a7897 (base 2fd13339, the head has not moved) — scripts/check-merge-plan-suite.py 2082 reads OK: 4422 passed, 298 skipped; check-merge-landing-diff.py 2082 shows the merge changes exactly 2 paths (emrg/server/daemon.py +9 −7, tests/test_daemon.py +44 −0).
Verified in both directions on this host. Forward — branch code plus its test: tests/test_daemon.py::test_the_daemons_kind_rule_is_the_layers_rule passes. Reverse — the pre-PR emrg/server/daemon.py restored (git checkout HEAD -- emrg/server/daemon.py) with the new test in place: it fails, with exactly the message the test was written for:
AssertionError: the daemon answered from its own copy of the rule rather than from the layer's reader
assert None == 'a sentinel kind'
So the guard is armed at the owner (emrg.tools.base.special_file_kind, which non_regular_kind reaches through its own module globals) rather than at the model, and a second spelling of the stat-then-name pair does not obey it — which is the thing issue #2081 is about.
Equivalence checked by hand as well: non_regular_kind(path) is special_file_kind(os.stat(path).st_mode), the same expression the wrapper restated, so the only difference is the one the docstring names — None when the stat itself fails, which _non_regular_kind's callers rely on because they ask the kind before any exists() test. special_file_kind has no remaining use in daemon.py after the import swap (the only other mentions are inside docstrings), so nothing is orphaned.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-081210
Reviewed on the tree it would land, not the PR page: check-merge-landing-diff.py 2082
reads landing tree 1585c798a759, which changes 2 paths on the base
(emrg/server/daemon.py +9 −7, tests/test_daemon.py +44 −0). The other 2 paths in
diff(base, head) are the base's own later commits (#2080), which the head does not
contain — reading them as this PR's change would review work it never touched.
check-merge-plan-suite.py 2082 on that tree: 4704 passed, 17 skipped (250 s).
The guard is armed at the rule's owner rather than at a model of it, which is the whole
point of the change. On the head, tests/test_daemon.py -k "kind_rule or named_pipe or regular_file" → 9 passed; restoring master's emrg/server/daemon.py over the head
leaves the new test as the only red — assert None == 'a sentinel kind' — so displacing
emrg.tools.base.special_file_kind really does move daemon._non_regular_kind's answer,
and a second copy of the stat-then-name pair would fail here. The None-rather-than-raise
difference at a path whose stat fails is pinned too, which is what keeps the wrapper from
being a bare re-export.
Cleanup rather than a behaviour fix — the two spellings agree today — and the drift it
closes is the one that would silently stop at the daemon's event-loop readers.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-082640
Landing tree 1585c798a75980b976118412e2d6ae19b93a7897 (base 2fd13339, the head has not moved) — scripts/check-merge-plan-suite.py 2082 reads OK: 4422 passed, 298 skipped; the landing diff changes exactly 2 paths (emrg/server/daemon.py +9 −7, tests/test_daemon.py +44 −0).
Verified in both directions on this host. Forward — branch code plus its test: test_the_daemons_kind_rule_is_the_layers_rule passes. Reverse — the pre-PR emrg/server/daemon.py restored with the new test in place: it fails on exactly the assertion it was written for (AssertionError: the daemon answered from its own copy of the rule rather than from the layer's reader). So the guard is armed at the owner (emrg.tools.base.special_file_kind, reached by non_regular_kind through its own module globals), not at a model that a coincidentally-agreeing copy could satisfy.
The equivalence is by hand too: non_regular_kind(path) is special_file_kind(os.stat(path).st_mode), the same expression the wrapper restated, so the only behaviour that changes is the one the docstring names — None when the stat itself fails, which _non_regular_kind's callers depend on because they ask the kind before any exists() test.
Closes #2081
daemon._non_regular_kindasked a path's kind by restating the tool layer's rule —path.stat().st_modefed tospecial_file_kind— while the layer already had a path-taking reader for exactly this,emrg/tools/base.non_regular_kind(added in123d0abc, which is also#2074's merge base, so it was available when the wrapper was written). One whitelist with two spellings drifts: a change to the layer's predicate would reach the three file tools and the memory store and stop at the daemon's readers, which are the ones that run on the event loop.What changed
emrg/server/daemon.py— the import moves fromspecial_file_kindtonon_regular_kind, and the wrapper becomes one line over it. The name, the signature and the answer are unchanged; only the place the rule is read from is.Nonewhen thestatitself fails._memory_index_compaction_noteand the two image readers ask the kind before anyexists()test, so a raise there would escape theexcept OSErrorthat wraps only the read which follows. The docstring says so.The guard is armed at the owner
tests/test_daemon.py::test_the_daemons_kind_rule_is_the_layers_ruledisplacesemrg.tools.base.special_file_kind— the rule's owner, whichnon_regular_kindreaches through its own module globals — and requiresdaemon._non_regular_kind's answer to move with it. A test comparing the two functions' outputs on a few fixtures would pass for a copy that happened to agree, which is the shape being closed; a second copy answers from the real predicate and fails. The same test pins theNone-vs-raise difference at a missing path.Verified in both directions, on the committed branch:
-k "kind_rule or named_pipe or regular_file");git show HEAD~1:emrg/server/daemon.py) makes it fail —AssertionError: the daemon answered from its own copy of the rule rather than from the layer's reader— so the guard discriminates rather than merely passing.Full suite on the branch: 4701 passed, 16 skipped in 4m16s (
uv run pytest tests/ -q);from emrg.client.app import run_clientandpython -m emrg --helpboth fine.Cleanup, not a bug fix: the two spellings agree today, so no behaviour was wrong. It is the drift that is closed, while
#2074's review is fresh.