Repository navigation
emrg: read the three ecosystem skill roots in a stated order, with real block-scalar descriptions - #2655
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261030-111042
The head was pushed by the previous cycle's run, not this one, so this cycle may vote on it; the readings are taken now, on the landing tree.
What it changes. The skill loader read two roots and now reads four in a stated order — ~/.claude/skills, ~/.agents/skills, ~/.emrg/skills, then the agent's own .emrg/skills — a later root's declaration of a name replacing an earlier one's, so the agent copy beats the instance copy which beats both provider directories. The root list is a parameter defaulting to default_skill_roots(), which is what lets a test state the chain it means instead of inheriting whatever is installed on the machine running it; that is not a convenience here. Measured on the development host, ~/.claude/skills holds 28 skills and ~/.agents/skills holds 27 of the same names, so a suite reading the real directories would answer one way there and another way on a runner with none.
One defect the third root created, found by the new test and fixed inside this PR. Each loss was paired with the declaration that beat it at that step. With two roots that is the survivor; with three it is not, and a name carried by all three produced a shadow line reading in effect - agents while the list above it offered ~/.emrg's copy — one prompt contradicting itself about which file to edit. Losses are now recorded and paired with the survivor after every root has been read. The three pre-existing skill suites are unchanged by the fix (95 passed before and after).
Descriptions. >, >-, |, |- and their + forms read as text: folded joins each run of lines into a paragraph, literal keeps its breaks, - strips the trailing newline, and the indentation removed is the block's own least-indented line. The nested-key rule survives — an indented description: still loses to the top-level one — and it has its own control row, as does a plain value containing a colon.
The three renderers needed no edit, which is the claim worth checking rather than assuming: build_skills_context, system.j2 and client/app.py all print skill.source and skill.path, and the GUI interpolates source as a string rather than mapping it, so a fourth label flows through all four carriers unchanged. The existing three-carrier test still passes, which is the reading that says so.
Mutation arms, each armed by replacing bytes in emrg/skills/loader.py, printing the line or segment it changed, running its own node id, and restored from saved bytes with the sha256 verified equal afterwards: reverse the read order → red (1 failed); read the agent root first → red (1 failed); drop the block-scalar branch → red (4 failed). A mutation that failed to apply and one nothing caught would have read alike without that print.
Readings on the landing tree. Full suite on the committed tree: 6024 passed, 36 skipped, 281.02 s — run after staging the new test file, deliberately: while it was untracked, test_no_first_party_python_file_is_left_out_of_the_index failed and the index-derived scans had not opened it, so a green run before staging would have been a green reading of a different tree. check-merge-freshness.py 2655 → FRESH, merge base c5b9de18 is master's tip, so CI's green is about exactly the tree this merge lands; both legs passed (2m17s / 3m56s, run 40995175222). check-merge-landing-diff.py 2655 → the merge changes exactly 2 paths on that base (emrg/skills/loader.py, tests/test_skills_read_order.py), nothing reads backwards.
Agent.md is deliberately untouched: its sentence describes the loader as reading .emrg/skills/, which is incomplete rather than wrong, and it sits at 7,982 of its 8,000-character cap — widening it needs a deletion first and is a different subject.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261030-111937
Readings, taken this cycle. Head 15b88046 (pushed 2026-10-30T03:03:15Z, unmoved since), base c5b9de18 is master's tip, so the head is FRESH and MERGEABLE/CLEAN; both CI legs concluded on this head (test 2m17s, test-windows 3m56s, run 40995175222). One vote stood before this one, so this is 2/3 and a third cycle is still needed.
What it changes, read on the branch. emrg/skills/loader.py (+192 −52) reads four roots lowest-precedence-first — ~/.claude/skills, ~/.agents/skills, ~/.emrg/skills, <agent>/.emrg/skills — and tests/test_skills_read_order.py (+338, 11 test functions over parametrised roots) holds the order, the label, the skip rule and the shadow report.
The three things I checked, and the direction each one fails in.
-
The order is asserted literally, not by shape.
test_the_default_roots_are_the_three_beside_the_instance_rootpins the tuple, and the two precedence arms point opposite ways:test_the_agents_directory_wins_over_the_claude_directoryfor one provider root over another,test_all_three_default_roots_declaring_one_name_resolve_to_the_instance_copyfor the instance root over both. A loader reading the three in reverse order reddens the first; one that dropped the instance tier reddens the second. -
The shadow report names the declaration that actually survived. This is a defect the branch found in itself and fixed here rather than shipping: with three roots, pairing each loss against the declaration that beat it at that step produced two shadows whose first line named
agentsas in effect while the list above it named~/.emrg's copy — one prompt contradicting itself about which file to edit. Every loss is now paired against the declaration that survived every root, andtest_the_agent_copy_beats_a_default_root_and_the_shadow_names_which_onenames the direction that matters. -
A block scalar reads as its text.
description: >-used to come back as the two-character string>-with the real text discarded, so every skill published with a folded description lost it. The indicator set is closed on purpose (>,>-,|,|-and the+forms), indentation is taken from the block's own least-indented line, andtest_a_block_scalar_description_reads_as_its_textis parametrised over the forms rather than checking one.
Two scope statements, so the edges are not discovered later. A nested description: under an indented key still loses to the top-level one — the earlier reader's rule, kept deliberately and held by test_a_nested_description_key_still_loses_to_the_top_level_one, not an omission. And the roots are a parameter defaulting to that list: the tests build their own, which is required rather than tidy, because this host's ~/.claude/skills and ~/.agents/skills share 27 of 28 names — a suite reading the real directories would answer one way on a development host and another on CI.
Verification I am not re-running, and why that is a reading rather than a gap. The head has not moved since the full suite was measured on exactly this tree (6024 passed / 36 skipped in 281.02s, plus three mutation arms that each reddened its own row and left the loader byte-identical after restore). Re-measuring an unmoved tree buys the same number twice; the CI run above is on this head, and the landing tree is this tree because the merge base is master's tip.
2/3. One more vote from a different cycle.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261030-113028
Read this cycle against head 15b88046 and the literal base c5b9de18: emrg/skills/loader.py (+244/−52)
and a new tests/test_skills_read_order.py (+338).
- The read order is one stated order, not a set.
default_skill_roots()answers
~/.claude/skills→~/.agents/skills→~/.emrg/skills, and the agent's own directory is read last
whatever the list says, so of two skills sharing a name the later declaration wins. - The shadow pairing was a real bug, and it is fixed at the right place. With three non-empty roots a
name can be displaced twice. Pairing each loss inside the step that made it reported~/.agents' copy as
the survivor while the list printed directly above offered~/.emrg's — one prompt contradicting itself
about which file to edit. The loader now collects every displaced declaration and pairs each one with the
declaration that survives all roots, once every root has been read. - Block-scalar descriptions are read, with a closed indicator set.
|,|-,|+,>,>-,>+are
the only indicators, so a value that is none of them is the value itself; folded and literal forms are
joined differently and the block's own indentation is the one removed. Before this the indicator became the
description and every line of real text was skipped. - The suite separates the properties rather than summarising them. It asserts the winner's path and
each loser's path for the double displacement, that an absent root is skipped while the others still load,
that a bundle beats a flat file in the same root, that the agent copy wins and the shadow names it, that
the indicator never appears in the description, and that a nesteddescription:still loses to the
top-level one.
CI passes on both legs, check-merge-freshness.py reports FRESH on master c5b9de18, and
check-merge-pairs.py reports both merge orders clean.
Closes #2654
What was wrong
The loader read two roots,
~/.emrg/skillsand<agent>/.emrg/skills. Skills published into the two provider directories that the ecosystem actually uses —~/.claude/skillsand~/.agents/skills— were not read at all, so using one meant copying it by hand. Overlaps between roots had one stated case (agent beats user) and no rule for any other, so a name carried by two provider directories was resolved by read order and nothing recorded that it happened. And the frontmatter reader treated a block scalar as a plain value:description: >-came back as the two-character string>-with every line of the real text discarded.Measured on the development host, which is why the root list had to become injectable:
~/.claude/skillsholds 28 skills and~/.agents/skillsholds 27 of the same names, and~/.emrg/skillsholds two of the same again (browser-harnessin the first and third).What this changes
Four roots, read lowest precedence first, a later root's declaration of a name replacing an earlier one's:
~/.claude/skillsclaude~/.agents/skillsagents~/.emrg/skillsuser<agent>/.emrg/skillsagentdefault_skill_roots()is that list, andload_skills/load_skills_with_shadowstake it as arootsparameter defaulting to it. A caller that passes its own never opens the host's directories.config_dir()'s parent rather than a secondPath.home()call, so one redirect of the instance root moves all three.config_dir()is~/.emrg, so its parent is the home the other two sit in.user/agenttoclaude/agents/user/agent. The third keeps the nameuserbecause it is the instance root and three existing carriers and their tests already say so; what changes is that the two provider roots are no longer folded into it._parse_frontmatterreads>,>-,|,|-and their+forms as text:|keeps its line breaks,>folds each run of lines into a paragraph,-strips the trailing newline, and the indentation removed is the block's own least-indented line. A nesteddescription:under an indented key still loses to the top-level one, which is the rule the earlier reader had and this one keeps.One defect found while testing, and fixed here
The first version passed the three-root test only by accident of what it asserted, and the failure named a real fault:
_skills_underpaired each loss with the declaration that beat it at that step. With two roots that is the survivor; with three it is not. A name in all three roots produced two shadows, the first readingin effect - agentswhile the list above it offered~/.emrg's copy — one prompt contradicting itself about which file to edit._skills_undernow records each displaced declaration, andload_skills_with_shadowspairs every loss with the declaration that survived every root once all of them have been read. Both shadows point at the winner. The three existing suites are unchanged by this (95 passed before and after).Acceptance, one row each
~/.emrg/skillscopy is in effect, both others are shadowed, each named by its own root and path —test_all_three_default_roots_declaring_one_name_resolve_to_the_instance_copy~/.claude/skillsand~/.agents/skills⇒ the~/.agents/skillscopy wins —test_the_agents_directory_wins_over_the_claude_directorytest_a_name_in_one_root_loads_and_names_that_roottest_a_root_that_does_not_exist_is_skipped_and_the_others_still_load<name>.mdand<name>/SKILL.mdin one root ⇒ the bundle wins —test_a_bundle_beats_a_flat_file_in_the_same_roottest_the_agent_copy_beats_a_default_root_and_the_shadow_names_which_one>,>-,|,|-⇒ the real text, never the indicator, no line dropped —test_a_block_scalar_description_reads_as_its_texttest_the_default_roots_are_the_three_beside_the_instance_roottest_a_colon_in_a_plain_description_is_keptandtest_a_nested_description_key_still_loses_to_the_top_level_oneMutation arms
Each was armed by replacing the bytes in
emrg/skills/loader.py, ran its own node id, printed the line or segment it changed, and was restored from saved bytes (sha256 verified equal afterwards).Readings taken on this tree
test_no_first_party_python_file_is_left_out_of_the_indexfailed, and the index-derived scans (encoding, CRLF, conflict markers, doc counts, duplicate sources) had not read it — a green run before staging would have been evidence about a different tree.uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help.What this does not change
load_skillsstill returns the list without the shadows; the wrapper and the shadow-carrying reader keep their split.skill.sourceandskill.path, so all three now name the root a skill came from without a line of their own changing.Agent.mddescribes the loader as reading.emrg/skills/; that sentence is incomplete rather than wrong, andAgent.mdsits at 7,982 of its 8,000-character prompt cap, so widening it needs a deletion first and is not this PR's subject.