Skip to content

emrg: a skill declaration that loses its name is named, not swallowed - #2564

Merged
argszero merged 1 commit into
masterfrom
fix/shadowed-skill-declaration
Oct 24, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/shadowed-skill-declaration

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2563

What was wrong

emrg/skills/loader.py keys a skill by two different things on its two layers. _skill_files_in selects candidate files by filename (flat[entry.stem], bundled[entry.name]) and returns them sorted by that key; _skills_under then files the parsed result under the declared name: (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_context renders the survivors only, and the single trace was a logger.debug line 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.md and z.md, both declaring name: dup → 1 skill, z.md's. Reversing the creation order gives the same answer, so filename order decides, not creation order.
  • alpha/SKILL.md and beta/SKILL.md, both declaring name: shared-name → 1 skill, beta's.
  • a user skill and an agent skill both declaring name: deploy → 1 skill, the agent's — the documented cross-root precedence.

What changed

  • emrg/skills/loader.py: _skills_under records every replacement as a SkillShadow (the name, the declaration in effect, the declaration it displaced). load_skills_with_shadows() returns (skills, shadows), and load_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 sets ctx["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 ## Skills section gains a "Shadowed names" paragraph naming both files. No new ## heading, so tests/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_context has 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 by system.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

  • One test per shape the issue names — flat/flat in one root, bundle/bundle in one root, user/agent — plus a control asserting that a tree with no collision reports no shadow, plus one test at the prompt level and one comparing the two carriers.
  • Three mutation arms, each isolating one carrier, all run on this branch:
    • reverting daemon.py + system.j2 reddens both prompt-level tests ("system.j2 does not name the collision");
    • disabling the helper's rendering reddens the other side ("build_skills_context does not name the collision");
    • disabling the recording reddens the three shape tests and the context test, while the control still passes — so the control is not passing because nothing is measured.
  • Full suite on this branch (this checkout, macOS): 5833 passed, 36 skipped in 274.21s. The eight added tests account for the difference from the 5825 this content carried. An earlier 5825/37 reading came from a temporary landing-tree worktree, a different environment, and its skip count is not compared with this one.

CI is pending.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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: dup renders - **dup**: in effect - user, …/z.md; shadowed - user, …/a.md;
  • two bundles, alpha/SKILL.md and beta/SKILL.md, both declaring name: shared-name renders … 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 names block at all;
  • the documented cross-root precedence is unchanged — the agent's deploy is 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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).

@argszero
argszero merged commit 262f2e1 into master Oct 24, 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.

emrg: two skills that declare one name silently become one, and the loser is named nowhere

2 participants