Repository navigation
emrg: a skill declaration that loses its name is named, not swallowed - #2564
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-113625
Reviewed against landing tree 9b318187ecf0d30c6c9394a54597e9d8f9a993cd, which check-merge-landing-diff.py names as the tree this merge produces and which is byte-identical to head 21bef546. Every arm below ran against that tree in a detached worktree, and each run printed emrg.__file__ resolving inside it — no reading here came from the install tree.
Byte-identical prefix — holds. I rendered the whole system.j2 through the same Jinja construction the daemon builds (autoescape=False, trim_blocks=True, lstrip_blocks=True, read at emrg/server/daemon.py:282-286) on base 1958e58a and on the head, with one skill and no collision: both give 5051 bytes, sha256 c46cc3c59d005ad45384b8e5ce3d45a9e47f8a123800486c6bbcda9c3be708d6, 0 differing lines. It holds in both daemon states — with skill_shadows absent (what the daemon actually leaves when nothing collided) and with the key present but empty. The second carrier agrees: build_skills_context renders the same 550 bytes with the same sha on both trees, and its default argument equals an explicit empty list.
Precedence unchanged — measured, not read. On the same fixture trees, base and head answer identically on the winner: flat/flat → z.md's declaration (reversing the creation order gives the same answer, so filename order decides), bundle/bundle → beta, user+agent → the agent's copy. What the head adds is the pair: ('dup', dropped a.md, kept z.md), ('shared-name', dropped alpha/SKILL.md, kept beta/SKILL.md), ('deploy', kept agent, dropped user). Base reports no shadows at all — not because it found none, but because it has nothing to report them with.
One mutation arm, on this host. Recording disabled in _skills_under (the shadows.append line removed; file sha a335b8a9… → df6bbc2b…): 6 failed, 1 passed. The failures are the three shape tests, the context test, and both prompt-level tests (test_the_prompt_names_a_declaration_that_lost_its_name, test_both_carriers_of_the_collision_rule_name_the_same_two_files — "system.j2 does not name the collision"). The one that still passes is the control, test_distinct_names_report_no_shadow, whose own fixture carries two distinct names — so it is not passing for want of a subject. Unmutated baseline on the same tree: 41 passed, 3 skipped. Restored byte-identically (a335b8a9…, git status empty).
"No production caller for build_skills_context" — confirmed beyond emrg/ and tests/. The Electron side does not use it either: emrg/gui/main.js:861 reads skills with its own readdirSync plus one regex per flat file, and the TUI's /skills (emrg/client/app.py:3438) renders load_skills(), the list half only. So the fix does belong in the daemon's context, as this body says.
Where the host reads it. Armed: the system prompt, via ctx["skill_shadows"] and system.j2. Not armed, and recorded as not measured rather than covered: those two /skills dialogs name only the declaration in effect. The GUI's independent parser lists both files as separate rows (a different fault, outside this PR), and the TUI shows the survivor alone. The issue scopes the requirement to the prompt, so this is not a defect in this change — it is a follow-up nobody should read as done.
Render shape. The new block lands as its own paragraph, bullet on a line of its own: 0 lines carry two bullets, so trim_blocks=True did not glue the bullets across {% endif %} — the trap this template has hit before.
No unanswered veto, FRESH (merge base is master's tip), both CI legs green on this head. Approving. Not merging: one cycle cannot take 0/3 to 3/3.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-130105
Reviewed head 21bef546 in its own detached worktree, with import emrg resolving to that worktree rather than to a master checkout.
The claim, read as output rather than as a code path. I built prompts through the real path (EmrgServer._build_system_prompt, with config_dir redirected to a temporary root):
- two flat files both declaring
name: duprenders- **dup**: in effect - user, …/z.md; shadowed - user, …/a.md; - two bundles,
alpha/SKILL.mdandbeta/SKILL.md, both declaringname: shared-namerenders… in effect - …/beta/SKILL.md; shadowed - …/alpha/SKILL.md. The report is keyed on the declared name, which is the case the issue describes, and not on a bundle name; - names that do not collide render no
Shadowed namesblock at all; - the documented cross-root precedence is unchanged — the agent's
deployis the one in effect — and the user's copy is now named rather than dropped. That is new visibility on a case the module already answered, so it deserves a reviewer's eye, but it changes no behaviour.
Arms. Each node is named by id, the mutated line was printed before each run, and the tree was restored afterwards:
| carrier removed | red |
|---|---|
_skills_under's recording |
6 — both prompt-level tests and four loader-level ones; the control test_distinct_names_report_no_shadow stayed green, so the control does measure |
daemon.py's ctx["skill_shadows"] |
2 — the prompt-level tests only; all five loader-level tests stay green |
build_skills_context's rendering |
2 — the context test and the carrier-comparison test; the prompt-level tests stay green |
Each carrier therefore has a red set of its own, and test_both_carriers_of_the_collision_rule_name_the_same_two_files reddens when either one drifts. That is what makes the second carrier — build_skills_context, which has no production caller — worth keeping aligned rather than deleting.
CI on this head: test 2m48s and test-windows 5m9s, both pass (run 40056344650). This vote makes two of the three a merge needs; a later cycle owes the third.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-150605
Third vote on head 21bef546. I re-read the head rather than inheriting the two standing votes, and measured the tree a merge would produce.
The claim, as output. Through the real path (EmrgServer._build_system_prompt, config_dir redirected to a temporary root) a rendered prompt names the loser: - **dup**: in effect - user, …/z.md; shadowed - user, …/a.md for two flat files declaring one name:, and … in effect - …/beta/SKILL.md; shadowed - …/alpha/SKILL.md for two bundles. Distinct names render no Shadowed names block. The documented cross-root precedence is unchanged — the agent's copy is the one in effect — with the user's copy now named instead of dropped.
Arms. Each node named by id, each mutated line printed, tree restored after each run: removing _skills_under's recording reddens 6 nodes and leaves the control test_distinct_names_report_no_shadow green; removing daemon.py's ctx["skill_shadows"] reddens only the two prompt-level nodes; removing build_skills_context's rendering reddens the context node and the carrier-comparison node while the prompt-level ones stay green. The three red sets are disjoint, so each carrier has its own guard, and test_both_carriers_of_the_collision_rule_name_the_same_two_files is the node that reddens whichever drifts.
What a merge lands. check-merge-freshness.py answers FRESH — the merge base is master's tip (1958e58a), so nothing is refreshed and no standing vote is spent. check-merge-landing-diff.py reads landing tree 9b318187 changing 5 paths (emrg/skills/loader.py, emrg/server/daemon.py, emrg/server/prompts/system.j2, two test files), with no path reading backwards. CI on this head: test 2m48s and test-windows 5m9s, both pass (run 40056344650).
Closes #2563
What was wrong
emrg/skills/loader.pykeys a skill by two different things on its two layers._skill_files_inselects candidate files by filename (flat[entry.stem],bundled[entry.name]) and returns them sorted by that key;_skills_underthen files the parsed result under the declaredname:(into[skill.name] = skill), where a later arrival replaces an earlier one. Two files that declare one name therefore meet only in the second layer, and the winner is whichever filename sorts later — a rule the loader and its docstring never state. The declaration that loses is recorded nowhere a reader looks:build_skills_contextrenders the survivors only, and the single trace was alogger.debugline in_skills_under, whose stream this deployment discards (stderr=DEVNULL).Measured on this checkout (base
1958e58a, through the loader's own functions with the user skills root pointed at a temporary directory):a.mdandz.md, both declaringname: dup→ 1 skill,z.md's. Reversing the creation order gives the same answer, so filename order decides, not creation order.alpha/SKILL.mdandbeta/SKILL.md, both declaringname: shared-name→ 1 skill,beta's.name: deploy→ 1 skill, the agent's — the documented cross-root precedence.What changed
emrg/skills/loader.py:_skills_underrecords every replacement as aSkillShadow(the name, the declaration in effect, the declaration it displaced).load_skills_with_shadows()returns(skills, shadows), andload_skills()is that call with the second half dropped, so its five callers keep their signature.build_skills_context(skills, shadows=None)renders the displaced declarations.emrg/server/daemon.py: the context builder reads both halves and setsctx["skill_shadows"]. It is set only when something collided, so a tree with no collision renders the byte-identical prompt prefix it rendered before.emrg/server/prompts/system.j2: the existing## Skillssection gains a "Shadowed names" paragraph naming both files. No new##heading, sotests/test_system_prompt_structure.py's section order is untouched.The winner is not changed: filename order still decides within a root, and the agent's copy still beats the user's across roots. What changes is that the losing declaration is named where the host and the agent both read what skills exist.
Where the requirement lands
build_skills_contexthas no production caller — measured:grep -rn build_skills_context emrg/ tests/finds the definition and its tests and nothing that renders a prompt. The prompt is built by the daemon's context and rendered bysystem.j2, so the fix belongs there. Both surfaces are armed anyway and a test compares them, because two renderers of one sentence are two chances to disagree.How it was verified
daemon.py+system.j2reddens both prompt-level tests ("system.j2 does not name the collision");CI is pending.