diff --git a/emrg/client/app.py b/emrg/client/app.py index 232c02b9..9fe204d9 100644 --- a/emrg/client/app.py +++ b/emrg/client/app.py @@ -3415,7 +3415,9 @@ async def handle_key(data: bytes) -> bool: await conn.send_command("skills_update") status.update(center="checking skill updates…") else: - skills = load_skills() + # The session's directory, not this process's: the project + # skills a session has are the ones under its own root. + skills = load_skills(Path(cwd) if cwd else None) if skills: lines = ["**Loaded Skills:**", ""] for s in skills: diff --git a/emrg/server/daemon.py b/emrg/server/daemon.py index ce9811ba..76198d11 100644 --- a/emrg/server/daemon.py +++ b/emrg/server/daemon.py @@ -786,10 +786,15 @@ def __init__(self, llm_config: LlmConfig) -> None: self.tools.register(SubmitRantTool()) logger.info("tools registered: %s", self.tools.names) - # Load skills - self.skills = load_skills() - if self.skills: - logger.info("skills loaded: %s", [s.name for s in self.skills]) + # Skills are **not** held here. A snapshot taken at startup cannot see a skill + # installed or edited later, and it was taken from the daemon's own working + # directory rather than the session's — so `load_skills` runs live, with the + # session's cwd, each time a system prompt is built and each time + # `skills_available` is answered (rant 2026-10-21T09:30:39). This line only + # records what the daemon's own directory sees, for the log. + startup_skills = load_skills() + if startup_skills: + logger.info("skills at startup: %s", [s.name for s in startup_skills]) async def serve(self) -> None: """Start listening for IPC connections (fixed-port, platform-adaptive).""" @@ -1224,8 +1229,9 @@ async def _skills_ttl_loop(self) -> None: while True: result = await run_update_check_once() if result.get("updated"): + # Nothing to refresh: the prompt reads the skills live, so the files + # this just rewrote are already what the next round sees. logger.info("skills auto-updated: %s", result["updated"]) - self.skills = load_skills() elif result.get("errors"): logger.warning("skills update errors: %s", result["errors"]) await asyncio.sleep(_UPDATE_TTL_SECONDS) @@ -2444,11 +2450,16 @@ def _build_system_prompt(self, session: Session | None = None) -> str: ctx["project_context"] = self._collect_project_context(session) # ── Skills ── - if self.skills: + # Read live, from **this session's** working directory: a skill installed, + # edited or deleted on disk has to reach the next prompt without a daemon + # restart, and a session's project skills are the ones under its own root + # rather than under the directory the daemon was started in. + skills = load_skills(session.cwd if session else None) + if skills: ctx["skills"] = [ {"name": s.name, "source": s.source, "path": s.path, "description": s.description} - for s in self.skills + for s in skills ] # ── Memory ── @@ -3160,7 +3171,11 @@ async def _process_message( # Installable-skills catalog (rant 2026-08-08T10:14:29): list # catalog skills with installed/managed status. entries = load_catalog_skills() - installed = {s.name for s in self.skills} + # Live, not a snapshot: the question is whether a skill is on disk *now*, + # which is the same reason the prompt reads them live. This frame carries + # no cwd, so the project half is the daemon's own directory — the catalog + # installs into the user root, which is read either way. + installed = {s.name for s in load_skills()} result = [ { "name": e.get("name", ""), @@ -3186,10 +3201,8 @@ async def _process_message( }) return result = await install_skill(name, confirmed=confirmed) - if result.get("ok"): - # Reload so the next system-prompt build includes the skill - # (design: "下次构建系统提示即含该技能", no daemon restart needed). - self.skills = load_skills() + # No reload step: the prompt reads the skills live, so the file this just + # wrote is what the next system-prompt build sees. await self._send(ws, {"type": "skills_install_result", "name": name, **result}) elif msg_type == "skills_update": diff --git a/emrg/skills/loader.py b/emrg/skills/loader.py index bdc2df6e..6c70b542 100644 --- a/emrg/skills/loader.py +++ b/emrg/skills/loader.py @@ -1,8 +1,18 @@ """Skill loader — parse Markdown skills with YAML frontmatter. -Skills are filesystem-based Markdown packages. They live in: - - ~/.emrg/skills/*.md (user skills) - - .emrg/skills/*.md (project skills) +Skills are filesystem-based Markdown packages. They live in two roots: + + - ~/.emrg/skills/ (user skills) + - /.emrg/skills/ (project skills) + +and each root is read in **both** layouts, `.md` and `/SKILL.md` +(`_skill_files_in`). `` is the nearest ancestor of the *session's* +working directory carrying a `.git` (`_project_root`) — not the process's cwd, +which for the daemon is `~/.emrg` and would give every session one project's +skills. + +Nothing here is cached: every `load_skills` call re-reads the files, so a skill +written, edited or deleted on disk reaches the next prompt without a restart. Each skill file has YAML frontmatter: --- @@ -31,6 +41,11 @@ # boundary is one fact, and a second spelling of it is a second boundary that can drift. from emrg.tools.base import non_regular_kind +# Where this instance's user skills live is `config_dir()/skills`, the same directory the +# installer publishes into and the catalog is read from — one derivation, three readers, +# and a test can redirect it in one place (`installer._resolve_dest` already does). +from emrg.config import config_dir + logger = logging.getLogger(__name__) @@ -153,34 +168,109 @@ def _parse_skill_file(file_path: Path, source: str) -> Optional[Skill]: ) -def load_skills(project_dir: Optional[Path] = None) -> list[Skill]: - """Load all skills from user and project directories. +def _project_root(start: Path) -> Path: + """The nearest ancestor of ``start`` carrying a ``.git``, else ``start`` itself. + + A session is normally opened *inside* a project rather than at its top, while the + project's skills sit at the top — so the root has to be resolved rather than + assumed to be the directory the session happens to be in. A directory with no + repository above it is its own root, which is what a scratch or temp directory + wants and what keeps a skills directory beside the files being worked on usable. + """ + for candidate in (start, *start.parents): + if (candidate / ".git").exists(): + return candidate + return start + + +def _user_skills_root() -> Path: + """``~/.emrg/skills`` — the instance-wide root, taken from `config_dir()`.""" + return config_dir() / "skills" + + +def _skill_files_in(root: Path) -> list[Path]: + """Every skill file one root carries, in both layouts the ecosystem uses. + + A skill is either a flat ``/.md`` or a bundle ``//SKILL.md``. + Both must be read: the flat form is what this instance's installer publishes + (``installer.install_skill`` writes ``.md``) and what the shipped catalog is, + while the bundle is the layout the skill spec documents and the only one that can + carry the files a skill references beside it. - User skills (~/.emrg/skills/) are loaded first, then project skills. - Skills with duplicate names: project overrides user. + When one root carries both for the same name the **bundle wins**: it is the form + that can hold the rest of the skill, so the flat file beside it is a leftover copy + of the same skill rather than a second one. + + Only the shape is decided here. Whether an entry can be opened at all — a FIFO, a + socket, a file with no frontmatter — stays `_parse_skill_file`'s question, which is + why a flat entry is listed without being stat'ed first: a FIFO named `wedged.md` + must reach the kind check that skips it, not be filtered out here where the skip + would be silent (issue #2083). + """ + try: + entries = list(root.iterdir()) + except OSError: + logger.debug("skill: cannot list %s", root) + return [] + + flat: dict[str, Path] = {} + bundled: dict[str, Path] = {} + for entry in entries: + try: + is_dir = entry.is_dir() + except OSError: + continue + if is_dir: + body = entry / "SKILL.md" + if body.is_file(): + bundled[entry.name] = body + elif entry.name.endswith(".md"): + flat[entry.stem] = entry + + candidates = dict(flat) + candidates.update(bundled) + return [candidates[name] for name in sorted(candidates)] + + +def _skills_under(root: Path, source: str, into: dict[str, Skill]) -> None: + """Read one root into ``into``, the later root winning on a shared name.""" + for skill_file in _skill_files_in(root): + skill = _parse_skill_file(skill_file, source) + if skill: + into[skill.name] = skill + logger.debug("skill loaded: %s (%s) from %s", skill.name, source, skill_file) + + +def load_skills(project_dir: Optional[Path] = None) -> list[Skill]: + """Load every skill from the user root and one project root, read live. + + User skills (``~/.emrg/skills/``) are read first, then the project's + (``/.emrg/skills/``); a skill of the same name: the project's wins. + + ``project_dir`` is the **session's** working directory — deliberately not the + process's, which for the daemon is whatever it was started in and made every + session read the same project's skills. It resolves to the nearest ancestor + carrying a ``.git`` (`_project_root`). Called with nothing, it falls back to the + process cwd, for a caller that has no session at all. + + Nothing is cached, here or above: every call re-reads the files, so a skill + installed, edited or deleted on disk reaches the next prompt without a restart. + The cost is one directory listing and a few small reads (0.074 ms median measured + for three skills), and the result is a deterministic function of the files — an + unchanged tree renders a byte-identical prompt prefix, so the provider's cache + still matches. """ - if project_dir is None: - project_dir = Path.cwd() + start = Path(project_dir) if project_dir is not None else Path.cwd() + project_root = _project_root(start) all_skills: dict[str, Skill] = {} + user_root = _user_skills_root() + if user_root.is_dir(): + _skills_under(user_root, "user", all_skills) - # User skills (lower priority) - user_dir = Path.home() / ".emrg" / "skills" - if user_dir.is_dir(): - for md_file in sorted(user_dir.glob("*.md")): - skill = _parse_skill_file(md_file, "user") - if skill: - all_skills[skill.name] = skill - logger.debug("skill loaded: %s (user)", skill.name) - - # Project skills (higher priority — override user) - proj_dir = project_dir / ".emrg" / "skills" - if proj_dir.is_dir(): - for md_file in sorted(proj_dir.glob("*.md")): - skill = _parse_skill_file(md_file, "project") - if skill: - all_skills[skill.name] = skill - logger.debug("skill loaded: %s (project)", skill.name) + project_skills_root = project_root / ".emrg" / "skills" + if project_skills_root.is_dir(): + _skills_under(project_skills_root, "project", all_skills) return sorted(all_skills.values(), key=lambda s: s.name) diff --git a/tests/test_skills_read_live.py b/tests/test_skills_read_live.py new file mode 100644 index 00000000..673c12c7 --- /dev/null +++ b/tests/test_skills_read_live.py @@ -0,0 +1,275 @@ +"""Skills are read live, per session, from both roots and in both layouts. + +Three defects, measured on `5c9ed63b` before this file existed (rant +2026-10-21T09:30:39): + +1. `EmrgServer.__init__` kept `self.skills`, a **snapshot** taken at daemon start and + refreshed only by three unrelated events. A skill written to `~/.emrg/skills/` + after start reached no prompt: measured in one session, `subagent-delegate` was + written at 09:13 and a later prompt carried **0** occurrences of it while + `skill-catalog`, loaded from the same directory by the same loader, was there. +2. `load_skills()` fell back to `Path.cwd()` — the **daemon's** directory, `~/.emrg` — + so every session's "project" skills were `~/.emrg/.emrg/skills/` whatever project + the session was in. +3. Both roots were walked with `glob("*.md")`, so a skill written as the spec's + `/SKILL.md` was invisible: `~/.emrg/skills/browser-harness/SKILL.md` + (7,940 B, with `references/` beside it) had never been loaded. + +Each test below names a property, not a line: what a session must see, and when. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path + +import pytest + +from emrg.config import LlmConfig +from emrg.server.daemon import EmrgServer +from emrg.session import Session +from emrg.skills import loader as loader_mod +from emrg.skills import registry as registry_mod +from emrg.skills.loader import _project_root, load_skills + +SKILL_TEXT = "---\nname: {name}\ndescription: {description}\n---\n\n# {name}\n\n{body}\n" + + +def _skill(name: str, description: str = "a skill", body: str = "Body.") -> str: + return SKILL_TEXT.format(name=name, description=description, body=body) + + +@pytest.fixture +def roots(tmp_path, monkeypatch): + """A user root and a project root, both off the real home (and both empty).""" + data = tmp_path / "home" / ".emrg" + (data / "skills").mkdir(parents=True) + monkeypatch.setattr(loader_mod, "config_dir", lambda: data) + monkeypatch.setattr(registry_mod, "config_dir", lambda: data) + + project = tmp_path / "project" + (project / ".git").mkdir(parents=True) + (project / ".emrg" / "skills").mkdir(parents=True) + return data / "skills", project + + +# ── the two layouts ──────────────────────────────────────────────── + + +def test_a_flat_file_and_a_directory_bundle_both_load(roots): + """`.md` and `/SKILL.md` are the same kind of skill, read alike.""" + user_root, _ = roots + (user_root / "flat-skill.md").write_text(_skill("flat-skill"), encoding="utf-8") + (user_root / "bundle-skill").mkdir() + (user_root / "bundle-skill" / "SKILL.md").write_text( + _skill("bundle-skill", "a bundled skill"), encoding="utf-8" + ) + + names = [s.name for s in load_skills(project_dir=user_root)] + assert names == ["bundle-skill", "flat-skill"], names + + +def test_a_bundle_carries_the_directory_its_references_sit_in(roots): + """The path a prompt prints has to lead a reader to the skill's own files. + + A bundle may ship `references/`, `scripts/` and the like beside `SKILL.md`; the + path recorded for the skill is that `SKILL.md`, whose parent is the bundle — so a + reader told to follow the skill can find the rest of it. + """ + user_root, _ = roots + bundle = user_root / "with-files" + (bundle / "references").mkdir(parents=True) + (bundle / "SKILL.md").write_text(_skill("with-files"), encoding="utf-8") + (bundle / "references" / "install.md").write_text("details\n", encoding="utf-8") + + skill = next(s for s in load_skills(project_dir=user_root) if s.name == "with-files") + assert skill.path.name == "SKILL.md" + assert skill.path.parent == bundle + assert (skill.path.parent / "references" / "install.md").is_file() + + +def test_one_name_in_both_layouts_loads_once_and_the_bundle_wins(roots): + """A root carrying both forms holds one skill, and the bundle is the one kept.""" + user_root, _ = roots + (user_root / "both.md").write_text( + _skill("both", "the flat leftover"), encoding="utf-8" + ) + (user_root / "both").mkdir() + (user_root / "both" / "SKILL.md").write_text( + _skill("both", "the bundle"), encoding="utf-8" + ) + + skills = [s for s in load_skills(project_dir=user_root) if s.name == "both"] + assert len(skills) == 1 + assert skills[0].description == "the bundle" + assert skills[0].path.name == "SKILL.md" + + +# ── the project root ─────────────────────────────────────────────── + + +def test_the_project_root_is_the_nearest_git_ancestor(roots): + """A session opened deep inside a project still sees that project's skills.""" + _, project = roots + (project / ".emrg" / "skills" / "project-skill.md").write_text( + _skill("project-skill"), encoding="utf-8" + ) + deep = project / "src" / "nested" + deep.mkdir(parents=True) + + assert _project_root(deep) == project + names = [s.name for s in load_skills(project_dir=deep)] + assert "project-skill" in names, names + + +def test_a_directory_with_no_repository_above_it_is_its_own_root(tmp_path): + """Scratch and temp directories are projects of their own, not somebody's child.""" + scratch = tmp_path / "scratch" + (scratch / ".emrg" / "skills").mkdir(parents=True) + (scratch / ".emrg" / "skills" / "here.md").write_text( + _skill("here"), encoding="utf-8" + ) + + assert _project_root(scratch) == scratch + + +def test_the_project_beats_the_user_root_on_a_shared_name(roots): + """Same name in both roots: the project's copy is the one loaded.""" + user_root, project = roots + (user_root / "shared.md").write_text( + _skill("shared", "the user copy"), encoding="utf-8" + ) + (project / ".emrg" / "skills" / "shared.md").write_text( + _skill("shared", "the project copy"), encoding="utf-8" + ) + + shared = next(s for s in load_skills(project_dir=project) if s.name == "shared") + assert shared.description == "the project copy" + assert shared.source == "project" + + +# ── nothing is cached ────────────────────────────────────────────── + + +def test_a_skill_written_after_a_load_is_seen_by_the_next_one(roots): + """Install, edit and delete all reach the next call without any reload step.""" + user_root, _ = roots + assert [s.name for s in load_skills(project_dir=user_root)] == [] + + (user_root / "later.md").write_text(_skill("later", body="first body"), encoding="utf-8") + assert "later" in [s.name for s in load_skills(project_dir=user_root)] + + (user_root / "later.md").write_text(_skill("later", body="second body"), encoding="utf-8") + later = next(s for s in load_skills(project_dir=user_root) if s.name == "later") + assert later.body.endswith("second body"), later.body + + (user_root / "later.md").unlink() + assert [s.name for s in load_skills(project_dir=user_root)] == [] + + +# ── the daemon reads with the session's directory ────────────────── + + +def _server(tmp_path: Path) -> EmrgServer: + """A daemon built the way the daemon tests build one.""" + import tempfile + + server = EmrgServer(LlmConfig(base_url="http://localhost", api_key="test")) + server._projects_log = Path(tempfile.mkdtemp()) / "projects.yml" + return server + + +def test_the_prompt_carries_the_sessions_project_skills(roots, tmp_path): + """The skills in a prompt come from the session's project, not the daemon's cwd.""" + _, project = roots + (project / ".emrg" / "skills" / "session-skill.md").write_text( + _skill("session-skill", "only this project has it"), encoding="utf-8" + ) + server = _server(tmp_path) + session = Session.create_with_id("skills-live", project / "src") + + rendered = server._build_system_prompt(session) + assert "session-skill" in rendered, "the session's own project skill did not reach the prompt" + + +def test_a_skill_installed_after_the_daemon_started_reaches_the_next_prompt(roots, tmp_path): + """The defect itself: a snapshot taken at start could never see this skill.""" + _, project = roots + server = _server(tmp_path) + + before = server._build_system_prompt(Session.create_with_id("skills-late-a", project)) + assert "late-skill" not in before + + (project / ".emrg" / "skills" / "late-skill.md").write_text( + _skill("late-skill", "written after the daemon started"), encoding="utf-8" + ) + + after = server._build_system_prompt(Session.create_with_id("skills-late-b", project)) + assert "late-skill" in after, "a skill on disk did not reach the next prompt" + + +def test_the_daemon_keeps_no_skills_snapshot(tmp_path): + """The snapshot is gone, and nothing may quietly reintroduce it. + + A `hasattr` check rather than a source grep: an attribute that exists again is the + same defect whatever code fills it, and the two live readers below reach disk. + """ + server = _server(tmp_path) + assert not hasattr(server, "skills"), ( + "the daemon holds a skills snapshot again; skills are read live with the " + "session's cwd, so a copy on the server can only go stale" + ) + + +# ── skills_available reads disk too ──────────────────────────────── + + +class _FakeWriter: + """The daemon reaches its client through `ws.send`; nothing else is recorded.""" + + def __init__(self) -> None: + self._frames: list[bytes] = [] + + async def send(self, data) -> None: + self._frames.append(data.encode() if isinstance(data, str) else data) + + +def _drive(server: EmrgServer, msg: dict) -> list[dict]: + writer = _FakeWriter() + asyncio.run(server._process_message(msg, writer)) # type: ignore[arg-type] + return [json.loads(raw) for raw in writer._frames] + + +def test_skills_available_answers_from_disk_not_a_start_time_snapshot(roots, tmp_path): + """The catalog's installed flags follow the files, exactly as the prompt does.""" + user_root, _ = roots + (user_root / "skill-catalog.md").write_text( + "---\n" + "name: skill-catalog\n" + 'description: "installable skills"\n' + "skills:\n" + " - name: installable-one\n" + ' description: "not installed yet"\n' + " repo: example/installable-one\n" + " install: self-publishing\n" + " dest: ~/.emrg/skills/\n" + " check: github_release\n" + "---\n\n# Catalog\n", + encoding="utf-8", + ) + server = _server(tmp_path) + + first = _drive(server, {"type": "skills_available"}) + entries = first[0]["skills"] if first and "skills" in first[0] else [] + assert [e["installed"] for e in entries if e["name"] == "installable-one"] == [False] + + (user_root / "installable-one.md").write_text( + _skill("installable-one", "now it is here"), encoding="utf-8" + ) + + second = _drive(server, {"type": "skills_available"}) + entries = second[0]["skills"] if second and "skills" in second[0] else [] + assert [e["installed"] for e in entries if e["name"] == "installable-one"] == [True], ( + "skills_available still reports a skill installed after start as missing" + )