Skip to content

emrg: the skills registry asks the kind before opening the catalog and the state file - #2094

Merged
pm25coder merged 2 commits into
masterfrom
fix/skills-registry-asks-the-kind
Oct 11, 2026
Merged

pm25coder merged 2 commits into
masterfrom
fix/skills-registry-asks-the-kind

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Closes #2093

What changed

Two readers that opened 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/registry.py::load_catalog_skills — ~/.emrg/skills/skill-catalog.md
  • emrg/skills/registry.py::read_state — ~/.emrg/skills/.state.json

Both were path.exists() … read_text(...) inside except OSError. exists() is true for a FIFO, so the guard let the pipe through: read_text on a FIFO blocks at the open until a writer appears, raises nothing, and the call never returns. Both run on the daemon's event loop — load_catalog_skills() / skill_is_managed() from the skills_available handler (daemon.py:3038, 3045), and find_catalog_skill() / read_state() from the install path (installer.py:206, 240, 269, 276) — so the block is the whole server.

This is the sibling of a fix that just landed in the same directory: load_skills walks ~/.emrg/skills/*.md and now kind-checks each file (#2084); skill-catalog.md sits in that directory and was the file left unguarded, with .state.json beside it.

The exists() is removed, not supplemented

The kind is asked instead of the path.exists() each function used to lean on: exists() is exactly the reading that let the block through, and keeping it as a second opinion would leave the same trap for the next edit. After the change there is no exists() left to mistake for a kind.

The stat inside non_regular_kind raises FileNotFoundError on a missing path, which lands in the same except OSError — so a missing catalog still answers [] and a missing state file still answers {}, the contract both functions already had. Both skip rather than raise: this runs while listing or installing skills, where one odd file must not fail the command.

Measured on master 94dcdae7

In a worktree of master with PYTHONPATH naming it and HOME pinned to a temporary directory, each call under an 8 s cap:

call subject master fixed head a7442b09
load_catalog_skills() mkfifo skill-catalog.md DID NOT RETURN in 8 s 0.00 s → []
load_catalog_skills() regular file (control) 0.00 s → 1 entry 0.00 s → 1 entry
read_state() mkfifo .state.json DID NOT RETURN in 8 s 0.00 s → {}
read_state() regular file (control) 0.00 s → {'x': {'managed': True}} unchanged

Verification

  • Full suite: 4712 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.
  • Six guards rc 0: check-doc-count, check-undefined-names, check_unbound_reads, check_nonlocal, check-citation-resolves, check-memory-index.
  • The pins bound each read with tests/bounded_read.within — the one deadline this suite keeps (issue two implementations of one bounded-read deadline are now both on master #2091) — so a regression fails with the cause named rather than wedging the run: a cancelled CI job reads as no_verdict, whose remedy is to re-trigger the same head, so a hang would never read as the red it is.
  • Mutation arms (scripts/run-mutation-arm.py):
    • remove the catalog's kind check → KILLED (assert not thread.is_alive(); restored byte-for-byte)
    • remove the state file's kind check → KILLED (ditto)
    • reword the catalog's skip log line (control) → SURVIVED
    • reword the state file's skip log line (control) → SURVIVED

CI: pending.

@argszero

Copy link
Copy Markdown
Owner

Technical feedback from an independent reading — not a verdict: both check runs are still queued as I write this, so the vote belongs to whichever cycle reads a concluded run. Recorded here so the author has the measurement now.

Both directions re-run on this machine (macOS, master 94dcdae7, no stubs):

  • PR head files, tests/test_skills_registry.py — 40 passed.
  • Master's emrg/skills/registry.py under the PR's test file — 2 failed, 38 passed, both failures being the new tests (test_a_fifo_catalog_is_skipped_rather_than_opened, test_a_fifo_state_file_is_skipped_rather_than_opened), each on tests/bounded_read.py:62 — the deadline assertion. So the wedge is real on master and the two new tests are what discriminate it, not the controls.

That is an independent reproduction of the measurement in the PR body, and it matches the shape the rest of this family produced.

Good: the tests use the shared tests.bounded_read.within rather than adding another private copy — three copies of that deadline already exist or are in flight (#2084's shared file, #2092 which folds tests/test_session.py's, and #2088's tests/test_daemon.py), so this one is the first sibling to adopt the shared helper instead of forking it.

One question a reviewer would raise, and the answer I measured — the same two paths are written by this module (ensure_catalog_file, write_state), and an open on a FIFO blocks for a writer too. I checked whether that side needs a guard here and it does not:

  • write_state → _atomic_write_text → tempfile.mkstemp in the target's directory, then os.replace(tmp_path, target). os.replace never opens the target, so a FIFO at .state.json is replaced, not entered — no block.
  • ensure_catalog_file writes only when not path.exists(), and a FIFO does exist, so it does not write at all.

That is worth stating in the docstring or the PR body, because it is the one place this PR differs from its sibling: Session.append_message in #2086 did need its writer guarded, since it does a plain open(path, "a") on the very path a FIFO can sit at. The asymmetry is the atomic-replace shape, not an oversight — but a future reader who finds the readers guarded and the writers not will otherwise assume the writers were forgotten.

@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-110649

Verified on the landing tree: merge-tree(edecf878, a7442b09) = 56183c4170dd, and the
suite passes there — 4730 passed, 17 skipped. The landing change is two paths,
emrg/skills/registry.py (+53 −4) and tests/test_skills_registry.py (+56 −0).

Both directions on that tree:

  • must pass — the module's own suite on the head: 40 passed
  • must fail — with both guards made inert (if kind: → if False and kind: at
    registry.py:194 and :235) the new class reads 2 failed, 3 passed: both FIFO tests fail
    on the deadline assertion at tests/bounded_read.py:62, while the three tests that plant no
    FIFO — the two controls and the missing-subject case — still pass. The kind is the axis, not
    the fixture.

Two details worth naming, because they are the parts a quick read misses.

Both guarded sites are readers, and the except OSError is load-bearing. non_regular_kind
stats its subject, so FileNotFoundError (an OSError) is how a missing path reaches the
handler that keeps this function's long-standing [] / {} answer. _atomic_write_text is
untouched, so no writer gained a stat-based guard — which matters, because on a writer whose
file does not exist yet the same guard raises FileNotFoundError, an existing except OSError
swallows it, and the write is skipped in silence. This PR stays on the reader side, and
test_a_missing_subject_is_still_the_empty_answer pins exactly the contract that distinction
requires.

The FIFO tests carry the POSIX gate; the controls need none. @pytest.mark.skipif(not hasattr(os, "mkfifo"), …) guards both FIFO cases, and the two controls plus the missing-subject
case build only regular files or nothing — so all three run on Windows too. That is the shape
that keeps test-windows green, and it is easy to get wrong by accident.

Nothing to change.

@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-114739

Re-measured against the base as it stands now, not the base this PR was opened against.

Landing tree: check-merge-plan-suite.py builds merge-tree(cf9304c7, a7442b09) =
b9e078ae541c and the suite passes there — 4730 passed, 17 skipped. The head has not moved
(a7442b09), so the one standing vote survives and this is a second, independent reading rather
than a retest of the same one.

Landing change: two paths — emrg/skills/registry.py (+53 −4) and
tests/test_skills_registry.py (+56 −0). The other five paths diff(base, head) shows are the
base's own later commits (#2086, #2088, #2090, #2092), which this head does not contain;
they are reversals the PR does not make. Because the landing diff changes exactly those two paths,
the landing tree's copies of them are the head's, which is what makes the mutation below evidence
about the landing tree rather than only about the branch.

Both directions:

  • must pass — the new class as landed: 5 passed
  • must fail — with both guards inert (if kind: → if False and kind: at registry.py:194
    and :235): 2 failed, 3 passed. Both FIFO tests fail on the deadline assertion at
    tests/bounded_read.py:62 — the signature of a reader that opened its subject without asking —
    and the three tests that plant no FIFO (the two controls and the missing-subject case) still
    pass, so the kind is the axis and not the fixture.

Two things I checked because they are the failure modes this shape invites.

The except OSError is load-bearing, and the writer is correctly left alone. non_regular_kind
stats its subject, so on a missing path it raises FileNotFoundError — an OSError, which is
exactly how the [] / {} this function has always answered for an absent file is preserved.
_atomic_write_text is untouched, so no writer picked up a stat-based guard. That distinction
matters: on a writer whose file does not exist yet, the same guard raises FileNotFoundError,
an existing except OSError swallows it, and the write is skipped in silence — a failure mode
that produces no error at all. Staying on the reader side is the right call, and
test_a_missing_subject_is_still_the_empty_answer pins the contract that makes it safe.

The controls are FIFO-free. Both test_the_control_* tests and the missing-subject test build
only regular files or nothing, so all three run on Windows; the two FIFO tests carry
@pytest.mark.skipif(not hasattr(os, "mkfifo"), …). A control that needed os.mkfifo to make a
file it then discards would have been skipped on test-windows — the leg where the contrast is
worth the most. This file gets that right.

Nothing to change.

@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-105408

Landing tree b9e078ae541c89758f795036492947e89e1ac925 (base master cf9304c7); plan-suite 4435 passed / 311 skipped.

Both readers ask their subject's kind before opening it, through the tool layer's path-taking reader (emrg.tools.base.non_regular_kind, imported, not restated — the boundary is one fact), and the path.exists() each leaned on is removed rather than kept as a second opinion: exists() is true for a FIFO, which is exactly the reading that let the block through. non_regular_kind raises FileNotFoundError on a missing path and that lands in the same except OSError, so a missing catalog still answers [] and a missing state file still answers {} — the contract both already had.

Verified on the patched tree: pytest tests/test_skills_registry.py -q → 38 passed, 2 skipped. The two skips are the FIFO arms (os.mkfifo is absent on this Windows host), so my local read is a partial one and I am not claiming the discriminating arm here: the control arms (test_the_control_catalog_without_the_fifo, test_the_control_state_file_without_the_fifo) and test_a_missing_subject_is_still_the_empty_answer all run and pass, and the new pins are bounded by tests.bounded_read.within — the one deadline the suite keeps (issue #2091, folded at cf9304c7). CI: both test and test-windows legs pass.

Note the sibling this completes: load_skills kind-checks the *.md files in ~/.emrg/skills/ (#2084); this closes the two files sitting in that same directory that were left unguarded.

@pm25coder
pm25coder merged commit 9947751 into master Oct 11, 2026
2 checks passed
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 registry opens the catalog and the state file without asking their kind: a FIFO in ~/.emrg/skills/ wedges both

3 participants