Repository navigation
emrg: the skills registry asks the kind before opening the catalog and the state file - #2094
Conversation
|
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
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 One question a reviewer would raise, and the answer I measured — the same two paths are written by this module (
That is worth stating in the docstring or the PR body, because it is the one place this PR differs from its sibling: |
argszero
left a comment
There was a problem hiding this comment.
✅ 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:194and:235) the new class reads2 failed, 3 passed: both FIFO tests fail
on the deadline assertion attests/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
left a comment
There was a problem hiding this comment.
✅ 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:atregistry.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
left a comment
There was a problem hiding this comment.
✅ 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.
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#2081exists to remove a second spelling of it.emrg/skills/registry.py::load_catalog_skills—~/.emrg/skills/skill-catalog.mdemrg/skills/registry.py::read_state—~/.emrg/skills/.state.jsonBoth were
path.exists()…read_text(...)insideexcept OSError.exists()is true for a FIFO, so the guard let the pipe through:read_texton 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 theskills_availablehandler (daemon.py:3038, 3045), andfind_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_skillswalks~/.emrg/skills/*.mdand now kind-checks each file (#2084);skill-catalog.mdsits in that directory and was the file left unguarded, with.state.jsonbeside it.The
exists()is removed, not supplementedThe 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 noexists()left to mistake for a kind.The
statinsidenon_regular_kindraisesFileNotFoundErroron a missing path, which lands in the sameexcept 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
94dcdae7In a worktree of master with
PYTHONPATHnaming it andHOMEpinned to a temporary directory, each call under an 8 s cap:a7442b09load_catalog_skills()mkfifo skill-catalog.md[]load_catalog_skills()read_state()mkfifo .state.json{}read_state(){'x': {'managed': True}}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.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 asno_verdict, whose remedy is to re-trigger the same head, so a hang would never read as the red it is.scripts/run-mutation-arm.py):assert not thread.is_alive(); restored byte-for-byte)CI: pending.