Skip to content

emrg: ask the kind before opening, in the skills loader and the sessions index - #2084

Merged
pm25coder merged 2 commits into
masterfrom
fix/ask-the-kind-before-opening-skills-and-sessions
Oct 11, 2026
Merged

pm25coder merged 2 commits into
masterfrom
fix/ask-the-kind-before-opening-skills-and-sessions

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

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 #2081 exists to remove a second spelling of it.

  • emrg/skills/loader.py::_parse_skill_file — reads .emrg/skills/*.md and ~/.emrg/skills/*.md. Runs on the daemon's event loop: the skills_install branch, the skills auto-update loop, and startup (emrg/server/daemon.py:762, 1188, 3066), plus emrg/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_text on 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 → None so the walk keeps going; an unreadable skill → None so 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 3386f239

From a worktree of master with PYTHONPATH naming it — a file-based script otherwise imports the install copy at ~/.emrg/install:

carrier fixture reading
load_skills(project_dir) a mkfifo named wedged.md beside a usable good.md DID NOT RETURN in 8 s
list(_iter_project_sessions(project)) a mkfifo at s2/meta.json, with a real s1 beside it DID NOT RETURN in 8 s

Fixed 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::within runs 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, and scripts/check-merge-freshness.py classifies a head whose non-successful jobs are all cancelled as no_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

  • Full suite: 4696 passed, 27 skipped, 0 failed.
  • uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help — rc 0.
  • Seven guards rc 0: check-doc-count, check-undefined-names, check_unbound_reads, check_nonlocal, check-citation-resolves, check-memory-index.
  • Mutation arms (scripts/run-mutation-arm.py), four of them:
    • remove the loader's kind check → KILLED (both nodes failed on assert not thread.is_alive(); restored byte-for-byte)
    • remove the sessions index's kind check → KILLED (ditto)
    • reword the loader's skip log line (control) → SURVIVED
    • reword the sessions index's skip log line (control) → SURVIVED

CI: pending.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ 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 tree 0637db3c4366, 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 tree 0637db3c4366, suite OK: 4711 passed, 17 skipped in 251 s.
  • scripts/check-merge-freshness.py 2084 — STALE (head 7a67e400, base 3386f239, 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:

  1. The rule is read, not restated. Both sites import emrg.tools.base.non_regular_kind instead of spelling the whitelist a third time — the same move #2076/#2082 made, so there is one boundary and not three.
  2. The answer at each site is the one that site already gives. _read_meta_session_id returns None for 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_file returns None for 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.
  3. The missing-path contract is pinned, not narrowed — test_a_missing_meta_is_still_none. Worth having: non_regular_kind stats its subject and stat raises on an absent path, which is exactly the case a careless rewrite gets wrong.
  4. 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.
  5. The deadline is what makes a regression readable at all. A reader that blocks at the open() raises nothing, so try/except cannot 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). The skipif(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 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-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.json and <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_sessions and _iter_nested_sessions both call _read_meta_session_id (sessions_index.py:124, :148), and _parse_skill_file is the only read_text in the skills loader.
  • Skipping is these functions' own answer, not a harsher one: _read_meta_session_id already returns None for a missing or unparseable meta so the walk keeps going, and _parse_skill_file already returns None for 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's FileNotFoundError is caught by the index reader's existing except (json.JSONDecodeError, OSError) and by the loader's new local except OSError, which returns None with the same "cannot read" log line the read_text path gave before.
  • tests/bounded_read.py is not collected as a test module (its name decides that) and tests/__init__.py exists, so from 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.

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 skills loader and the sessions index open a path without asking its kind: a FIFO wedges both

3 participants