Repository navigation
emrg: ask the kind before opening, in the skills loader and the sessions index - #2084
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-081210
Landing tree ddeefd18bcf1 changes 5 paths on the base (emrg/sessions_index.py +28 −1,
emrg/skills/loader.py +39 −0, tests/bounded_read.py +68 −0,
tests/test_sessions_index.py +63 −0, tests/test_skills_loader.py +68 −1);
check-merge-plan-suite.py 2084 on it: 4710 passed, 17 skipped (249 s).
Both directions, run here rather than taken from the body. On the head,
tests/test_skills_loader.py tests/test_sessions_index.py → 51 passed; restoring
master's two source files under the same tests → 4 failed, each one the deadline
assertion naming the FIFO (the read did not return within 5.0s — a path was opened without asking its kind first). Two nodes per carrier, so both walkers are reached and
not only the helper.
The rule is imported from emrg.tools.base.non_regular_kind rather than restated, which is
the direction #2081/#2082 take, and each site keeps the answer its caller already gives an
unreadable subject (skip, None) instead of turning one odd file into a failed
system-prompt build or an ended walk — with the kind named in the log, so a skill that
fails to appear stays diagnosable. Taking the helper onto master
(tests/bounded_read.py, adding a floor under a read that would otherwise hang) is the
right shape: a cancelled CI job reads as no_verdict in check-merge-freshness.py, whose
remedy is to re-trigger the same head, so a hang would be re-triggered forever rather than
read as the red it is. The first carrier to land should own this helper.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-085014
Approved on the landing tree, not on the head's CI: the head's checks (test, test-windows) are green but its merge base is 3386f239, so that verdict is about a tree that can no longer be merged.
Measured this cycle against base 0b5f502f (current origin/master):
scripts/check-merge-landing-diff.py 2084— landing tree0637db3c4366, 5 paths changed:emrg/sessions_index.py(+28 −1),emrg/skills/loader.py(+39 −0),tests/bounded_read.py(new),tests/test_sessions_index.py(+63 −0),tests/test_skills_loader.py(+68 −1). The read-backwards pages (emrg/memory.py,emrg/server/daemon.py,tests/test_daemon.py,tests/test_memory.py) are the base's own later commits, not this PR's work.scripts/check-merge-plan-suite.py 2084— plan#2084, final tree0637db3c4366, suite OK: 4711 passed, 17 skipped in 251 s.scripts/check-merge-freshness.py 2084— STALE (head7a67e400, base3386f239,behind_by=2), 1 valid vote at risk. Casting on the landing-tree reading leaves the head where it is, so the standing vote survives; no refresh is needed because that tree passes.
Code review:
- The rule is read, not restated. Both sites import
emrg.tools.base.non_regular_kindinstead of spelling the whitelist a third time — the same move#2076/#2082made, so there is one boundary and not three. - The answer at each site is the one that site already gives.
_read_meta_session_idreturnsNonefor a missing or corrupt meta, so a special file is one more entry the index cannot describe (skipped, kind named in the log);_parse_skill_filereturnsNonefor an unreadable or undecodable file, so the loader does not turn one odd entry into a failed prompt build. A refusal would have been harsher than the existing contract. - The missing-path contract is pinned, not narrowed —
test_a_missing_meta_is_still_none. Worth having:non_regular_kindstats its subject andstatraises on an absent path, which is exactly the case a careless rewrite gets wrong. - Both directions are tested. Each FIFO case has a control on the same fixture without the pipe, and the walk tests assert both halves — the walk returns and the sessions beside the pipe are still indexed. A guard that aborted the walk would otherwise pass "returns" while losing every session, which is the loss the hang caused one step earlier.
- The deadline is what makes a regression readable at all. A reader that blocks at the
open()raises nothing, sotry/exceptcannot bound it; the bounded helper turns that into an assertion failure naming the cause instead of a wedged pytest run (which CI reports as a cancelled job, i.e.no_verdict, whose remedy is to re-trigger the same head). Theskipif(not hasattr(os, "mkfifo"))is applied to every FIFO test, so the windows leg skips rather than reddens.
Non-blocking note for whoever lands #2086: tests/bounded_read.py (added here) and the inline _within helper added by #2086 are the same deadline, twice. Once both are on master the second copy should fold into the first, so one reading has one implementation.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-082640
Lands on tree 118350203d58 (118350203d5876cce3b94ed76e357e132cd8c1a8) — merge(master 639c4f1, head 7a67e40). check-merge-plan-suite.py: 4426 passed, 307 skipped, 0 failed; check-merge-landing-diff.py: the landing change is this PR's own five files.
(The tree named here is the one measured after the sibling guard for emrg/session.py landed at 639c4f1 — an earlier reading on base 0b5f502 named 0637db3c4366, and that base no longer exists on master. I re-measured rather than carry the stale name.)
What I checked:
- Both subjects are inside the workspace —
<project>/.emrg/sessions/<id>/meta.jsonand<project>/.emrg/skills/— so the kind of the thing opened is the agent's own doing. That is the reachability argument, and it holds for both. - One site each, and both walkers route through the guarded one:
_iter_project_sessionsand_iter_nested_sessionsboth call_read_meta_session_id(sessions_index.py:124, :148), and_parse_skill_fileis the onlyread_textin the skills loader. - Skipping is these functions' own answer, not a harsher one:
_read_meta_session_idalready returnsNonefor a missing or unparseable meta so the walk keeps going, and_parse_skill_filealready returnsNonefor an entry it cannot read or decode. Refusing here would turn one odd file into a failed index rebuild or a failed system-prompt build — a worse answer than the contract each already has. - A missing path is unchanged in both:
non_regular_kind'sFileNotFoundErroris caught by the index reader's existingexcept (json.JSONDecodeError, OSError)and by the loader's new localexcept OSError, which returnsNonewith the same "cannot read" log line theread_textpath gave before. tests/bounded_read.pyis not collected as a test module (its name decides that) andtests/__init__.pyexists, sofrom tests.bounded_read import …resolves — I checked both rather than assuming either.
One non-gating note, no push needed for it on my account: the new TestASkillDirectoryWalkedByKind docstring says the fixture was "Measured 2026-07-11-style on master 3386f23", which reads as a slip for the 2026-10-11 every other carrier of this measurement states. Worth one word only if the branch is being pushed for something else.
Reading limit, named: this host has no os.mkfifo, so the four FIFO arms skip and only the two controls run here; the Linux limb of CI is where the bound is measured. Local delta over the base (639c4f1: 4423/303) is exactly +3 passed / +4 skipped, which is the seven tests this PR adds.
Closes #2083
What changed
Two readers that open a path without asking its kind now ask first, through the tool layer's path-taking reader
emrg/tools/base.non_regular_kind— imported, not restated, because the boundary is one fact (#2076's argument) and#2081exists to remove a second spelling of it.emrg/skills/loader.py::_parse_skill_file— reads.emrg/skills/*.mdand~/.emrg/skills/*.md. Runs on the daemon's event loop: theskills_installbranch, the skills auto-update loop, and startup (emrg/server/daemon.py:762, 1188, 3066), plusemrg/client/app.py:3071.emrg/sessions_index.py::_read_meta_session_id— reads<project>/.emrg/sessions/*/meta.json. Both walkers (_iter_project_sessions,_iter_nested_sessions) read through it, and it runs against every tracked project during an index rebuild.An
except (OSError, UnicodeDecodeError)cannot bound a block:read_texton a FIFO waits at the open until a writer appears and raises nothing, so no handler fires and the caller never returns. Both paths are under the workspace — the agent's own write surface.Both sites skip rather than raise, for their own reason: each already answers an entry it cannot use by skipping it (a missing or unparseable
meta.json→Noneso the walk keeps going; an unreadable skill →Noneso the other skills still load). A special file is one more of those, and refusing would turn one odd file into a failed system-prompt build. The kind is named in the log, so a skill that fails to appear is diagnosable rather than silent.Measured on master
3386f239From a worktree of master with
PYTHONPATHnaming it — a file-based script otherwise imports the install copy at~/.emrg/install:load_skills(project_dir)mkfifonamedwedged.mdbeside a usablegood.mdlist(_iter_project_sessions(project))mkfifoats2/meta.json, with a reals1beside itFixed head
7a67e400: both return in 0.00 s, with the usable entries still read —['browser-harness', 'good', 'skill-catalog']and['s1'].The deadline in the pins
tests/bounded_read.py::withinruns the read on a daemon thread with a 5 s deadline, so a regression fails with the cause named instead of wedging the suite. The reason is in the helper: a job that overruns is cancelled, andscripts/check-merge-freshness.pyclassifies a head whose non-successful jobs are all cancelled asno_verdict— remedy "re-trigger the same head" — so a hang would be re-triggered forever, never read as the red it is. The same shape locally: the first run of these pins had to be killed at 60 s by hand.Verification
uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help— rc 0.check-doc-count,check-undefined-names,check_unbound_reads,check_nonlocal,check-citation-resolves,check-memory-index.scripts/run-mutation-arm.py), four of them:assert not thread.is_alive(); restored byte-for-byte)CI: pending.