diff --git a/backend/apps/agents/agent_manager.py b/backend/apps/agents/agent_manager.py index 69aff20c..5fc572d0 100644 --- a/backend/apps/agents/agent_manager.py +++ b/backend/apps/agents/agent_manager.py @@ -828,6 +828,48 @@ class AgentManager: sections.append(f"[Using skill: {name}]\n\n{content}") return "\n\n".join(sections) + @staticmethod + def _resolve_sdk_skill_allowlist(attached_skills: list | None) -> list[str]: + """Build the per-turn SDK Skill-tool allowlist. + + The Claude Code CLI auto-discovers skills from `~/.claude/skills// + SKILL.md` and surfaces each one's frontmatter (name + description) into + the model's system prompt so it can decide on its own when to call + `Skill(slug)` and pull the body. The SDK exposes that filter as + `options.skills`: + + - `None` — CLI defaults (re-enables Claude Code's bundled plugin + skills like /init, /review, /security-review, ...). We don't want + those: half mutate ~/.claude state OpenSwarm doesn't read, and the + rest reference slash commands the backend never intercepts. + - `[]` — suppress everything (was the previous behavior; safe but + also kills user-installed skills, defeating the point). + - `list[str]` — allowlist. Bundled skills are filtered out because + they're not on the list, and the model still sees user skills. + + Manually-attached skills (the `/` picker → `attached_skills` flow) are + excluded from the allowlist for *this* turn: their full body is + already prepended to the user message by `_resolve_attached_skills`, + so admitting them on the lazy Skill-tool path too would just let the + model load the same content a second time. + + The installed-slugs scan is cached in `backend.apps.skills.skills` + and invalidated by the create / delete endpoints + the startup + migration, so this is effectively free on the hot path — no + per-turn filesystem walk. + """ + from backend.apps.skills.skills import get_installed_slugs + + installed = get_installed_slugs() + if not attached_skills: + return list(installed) + attached_ids = { + s.get("id") for s in attached_skills if s.get("id") + } + if not attached_ids: + return list(installed) + return [slug for slug in installed if slug not in attached_ids] + @staticmethod def _get_branch_messages(session) -> list: """Return the linear message list for the active branch, walking the branch tree.""" @@ -1921,20 +1963,23 @@ class AgentManager: "type": "preset", "preset": "claude_code", } - # Suppress Claude Code's bundled plugin skills (/init, /review, - # /security-review, /simplify, /loop, /schedule, /update-config, - # /keybindings-help, /fewer-permission-prompts, /claude-api). - # OpenSwarm has its own skills system that injects skill content - # into the user prompt via _resolve_attached_skills, bypassing the - # SDK's Skill tool entirely — so the bundled skills only cause - # confusion: half mutate ~/.claude state OpenSwarm doesn't use - # (settings.json, keybindings.json), and the rest tell the model - # it can invoke slash commands the backend doesn't intercept. - # Empty list is the SDK's documented "skills off" signal — see - # the `skills` field on ClaudeAgentOptions in claude_agent_sdk - # types.py. User-attached skills are unaffected because they - # don't go through the Skill tool. - options_kwargs["skills"] = [] + # Skill auto-discovery: hand the SDK an allowlist of the user's + # installed skill slugs (one dir per skill under ~/.claude/skills/) + # so the CLI exposes each one's frontmatter to the model and the + # model can call `Skill(slug)` on its own when relevant. Bundled + # Claude Code skills (/init, /review, /security-review, /simplify, + # /loop, /schedule, /update-config, /keybindings-help, + # /fewer-permission-prompts, /claude-api) are filtered out by + # virtue of *not* being on the allowlist — half mutate ~/.claude + # state OpenSwarm doesn't read and the rest reference slash + # commands the backend never intercepts. + # + # Skills the user attached this turn via `/` are dropped from the + # allowlist: their full body is already prepended to the user + # message by `_resolve_attached_skills`, so allowing the SDK + # tool path would let the model double-load the same content. + # See `_resolve_sdk_skill_allowlist` for the full rationale. + options_kwargs["skills"] = self._resolve_sdk_skill_allowlist(attached_skills) # exclude_dynamic_sections=True tells the CLI to keep # per-user/per-machine grounding (cwd, git status, recent # commits, OS info) out of the cached system prompt prefix diff --git a/backend/apps/skills/skills.py b/backend/apps/skills/skills.py index 0bfa0526..868aaded 100644 --- a/backend/apps/skills/skills.py +++ b/backend/apps/skills/skills.py @@ -1,4 +1,5 @@ import os +import shutil import json import logging import re @@ -19,12 +20,147 @@ from backend.config.paths import SKILLS_WORKSPACE_DIR async def skills_lifespan(): os.makedirs(SKILLS_DIR, exist_ok=True) os.makedirs(SKILLS_WORKSPACE_DIR, exist_ok=True) + _migrate_flat_to_dir_layout() yield skills = SubApp("skills", skills_lifespan) +# --------------------------------------------------------------------------- +# Installed-slugs cache +# --------------------------------------------------------------------------- +# `_run_agent_loop` asks for the list of installed skill slugs on *every* +# turn so it can hand the SDK an auto-discovery allowlist. The disk layout +# rarely changes mid-session, so re-walking SKILLS_DIR + statting each +# subdir's SKILL.md on every turn is wasted work — for N skills and T +# turns that's N*T syscalls in the agent hot path, all returning the same +# answer. +# +# Cache the slug list at module scope, key it by the current SKILLS_DIR +# (so test monkeypatching automatically forces a refetch when the dir +# moves), and invalidate explicitly from the create / delete endpoints +# and from the startup migration. Update doesn't invalidate because +# updates can only change a skill's content or metadata, never the set +# of slugs (`update_skill` 404s if the slug dir doesn't already exist). +# +# Concurrency: FastAPI runs handlers on the asyncio event loop in a +# single thread; none of the cache-touching code paths await between +# reading and writing `_INSTALLED_SLUGS_CACHE`, so no lock is needed. +# Multi-worker deployments each get their own cache, but they also have +# disjoint disk layouts being scanned, so that's fine. +_INSTALLED_SLUGS_CACHE: tuple[str, list[str]] | None = None + + +def _invalidate_installed_slugs_cache() -> None: + """Drop the cached slug list; next `get_installed_slugs` rescans.""" + global _INSTALLED_SLUGS_CACHE + _INSTALLED_SLUGS_CACHE = None + + +def get_installed_slugs() -> list[str]: + """Return sorted slugs of installed skills (`/SKILL.md`). + + Cached at module level and keyed by SKILLS_DIR so a test that + monkeypatches the dir gets a fresh scan automatically. The returned + list is shared — callers must not mutate it. + """ + global _INSTALLED_SLUGS_CACHE + cached = _INSTALLED_SLUGS_CACHE + if cached is not None and cached[0] == SKILLS_DIR: + return cached[1] + if not os.path.isdir(SKILLS_DIR): + result: list[str] = [] + else: + result = sorted( + entry + for entry in os.listdir(SKILLS_DIR) + if not entry.startswith(".") + and os.path.isfile(os.path.join(SKILLS_DIR, entry, "SKILL.md")) + ) + _INSTALLED_SLUGS_CACHE = (SKILLS_DIR, result) + return result + + +# Skills used to live as flat files (`~/.claude/skills/.md`) because +# OpenSwarm injected their full body into the user prompt itself and never +# leaned on Claude Code's own discovery. Now that we want the agent to +# auto-discover skills via the SDK's Skill tool, we have to match the CLI's +# convention, which is one directory per skill with a SKILL.md inside — +# confirmed by the bundled CLI's discovery code (`/SKILL.md`) and its +# user-facing docs ("`~/.claude/skills//SKILL.md`"). Anything else is +# silently ignored by the CLI. +def _migrate_flat_to_dir_layout() -> None: + """One-shot migration: move `.md` -> `/SKILL.md`. + + Idempotent: re-running after the move is a no-op. Crash-safe: the new + file is written before the old one is removed. If a directory already + exists for a slug it wins; we log and leave the flat file untouched so + the operator can resolve it manually. + """ + if not os.path.isdir(SKILLS_DIR): + return + for entry in os.listdir(SKILLS_DIR): + if not entry.endswith(".md") or entry.startswith("."): + continue + flat_path = os.path.join(SKILLS_DIR, entry) + if not os.path.isfile(flat_path): + continue + slug = entry[:-3] + target_dir = os.path.join(SKILLS_DIR, slug) + target_skill = os.path.join(target_dir, "SKILL.md") + if os.path.isdir(target_dir): + if not os.path.isfile(target_skill): + # Pre-existing dir without a SKILL.md is unusable; bail + # rather than overwrite whatever else is in there. + logger.warning( + f"Skill migration: {target_dir} exists but has no SKILL.md; " + f"leaving flat file {flat_path} in place." + ) + continue + logger.info( + f"Skill migration: directory {target_dir} already present, " + f"removing redundant flat file {flat_path}" + ) + try: + os.remove(flat_path) + except OSError as exc: + logger.warning(f"Skill migration: could not remove {flat_path}: {exc}") + continue + try: + with open(flat_path, encoding="utf-8") as f: + body = f.read() + except OSError as exc: + logger.warning(f"Skill migration: could not read {flat_path}: {exc}") + continue + index = _load_index() + meta = index.get(slug, {}) + body = _ensure_frontmatter( + body, + name=meta.get("name", slug.replace("-", " ").replace("_", " ").title()), + description=meta.get("description", ""), + ) + os.makedirs(target_dir, exist_ok=True) + with open(target_skill, "w", encoding="utf-8") as f: + f.write(body) + try: + os.remove(flat_path) + except OSError as exc: + logger.warning( + f"Skill migration: wrote {target_skill} but could not remove " + f"{flat_path}: {exc}" + ) + logger.info(f"Skill migration: {flat_path} -> {target_skill}") + # Migration may have changed the slug set; drop the cache so the + # first post-startup call (typically from `_run_agent_loop`) rescans. + _invalidate_installed_slugs_cache() + + +def _skill_path(slug: str) -> str: + """Canonical on-disk path for a skill's SKILL.md.""" + return os.path.join(SKILLS_DIR, slug, "SKILL.md") + + def _load_index() -> dict[str, dict]: if os.path.exists(INDEX_PATH): with open(INDEX_PATH) as f: @@ -33,40 +169,26 @@ def _load_index() -> dict[str, dict]: def _save_index(index: dict[str, dict]): + os.makedirs(SKILLS_DIR, exist_ok=True) with open(INDEX_PATH, "w") as f: json.dump(index, f, indent=2) -def _sync_skills() -> list[Skill]: - """Sync skills from the filesystem, updating the index.""" - index = _load_index() - result = [] - - if os.path.exists(SKILLS_DIR): - for fname in os.listdir(SKILLS_DIR): - if fname.endswith(".md"): - fpath = os.path.join(SKILLS_DIR, fname) - with open(fpath) as f: - content = f.read() - - skill_id = fname.replace(".md", "") - meta = index.get(skill_id, {}) - skill = Skill( - id=skill_id, - name=meta.get("name", fname.replace(".md", "").replace("-", " ").replace("_", " ").title()), - description=meta.get("description", ""), - content=content, - file_path=fpath, - command=meta.get("command", fname.replace(".md", "")), - ) - result.append(skill) - - return result - - -@skills.router.get("/list") -async def list_skills(): - return {"skills": [s.model_dump() for s in _sync_skills()]} +def _decode_yaml_scalar(raw: str) -> str: + """Inverse of `_yaml_scalar` — decodes the values it can emit. + """ + raw = raw.strip() + if len(raw) >= 2 and raw[0] == '"' and raw[-1] == '"': + try: + return json.loads(raw) + except json.JSONDecodeError: + return raw[1:-1] + if len(raw) >= 2 and raw[0] == "'" and raw[-1] == "'": + # YAML single-quote rule: interior `''` is a literal `'`. We + # don't emit single-quoted scalars ourselves, but hand-edited + # frontmatter sometimes does. + return raw[1:-1].replace("''", "'") + return raw def _parse_skill_frontmatter(raw: str) -> dict: @@ -81,10 +203,89 @@ def _parse_skill_frontmatter(raw: str) -> dict: for line in fm_block.splitlines(): m = re.match(r"^(\w[\w_-]*)\s*:\s*(.+)$", line) if m: - meta[m.group(1).strip()] = m.group(2).strip().strip('"').strip("'") + meta[m.group(1).strip()] = _decode_yaml_scalar(m.group(2)) return meta +def _ensure_frontmatter(content: str, *, name: str, description: str) -> str: + """Make sure SKILL.md starts with a YAML block carrying name+description. + + The CLI only surfaces a skill in the auto-discovery listing if its + frontmatter parses, so on every write we either patch the existing + block (preserving the user's other keys) or prepend a fresh one. + """ + existing = _parse_skill_frontmatter(content) + if existing: + end = content.find("---", 3) + body = content[end + 3 :].lstrip("\n") + else: + body = content + merged = {**existing, "name": name} + if description or "description" not in merged: + merged["description"] = description + fm_lines = [f"{k}: {_yaml_scalar(v)}" for k, v in merged.items() if v != "" or k in ("name", "description")] + return "---\n" + "\n".join(fm_lines) + "\n---\n\n" + body + + +def _yaml_scalar(value: object) -> str: + """Quote a YAML scalar only when needed (matches CLI parser tolerance).""" + s = str(value) + if s == "" or any(c in s for c in (":", "#", "\n")) or s.strip() != s: + return json.dumps(s) + return s + + +def _sync_skills() -> list[Skill]: + """Sync skills from the filesystem, updating the index. + + Walks `SKILLS_DIR//SKILL.md` (the layout the CLI discovers) and + pulls metadata from the file's YAML frontmatter, falling back to the + sidecar index for fields the user only set via the API. + """ + index = _load_index() + result = [] + + if not os.path.isdir(SKILLS_DIR): + return result + + for slug in os.listdir(SKILLS_DIR): + if slug.startswith("."): + continue + skill_dir = os.path.join(SKILLS_DIR, slug) + if not os.path.isdir(skill_dir): + continue + skill_path = os.path.join(skill_dir, "SKILL.md") + if not os.path.isfile(skill_path): + continue + with open(skill_path, encoding="utf-8") as f: + content = f.read() + frontmatter = _parse_skill_frontmatter(content) + meta = index.get(slug, {}) + name = ( + frontmatter.get("name") + or meta.get("name") + or slug.replace("-", " ").replace("_", " ").title() + ) + description = frontmatter.get("description") or meta.get("description", "") + result.append( + Skill( + id=slug, + name=name, + description=description, + content=content, + file_path=skill_path, + command=meta.get("command", slug), + ) + ) + + return result + + +@skills.router.get("/list") +async def list_skills(): + return {"skills": [s.model_dump() for s in _sync_skills()]} + + @skills.router.post("/workspace/seed") async def seed_skill_workspace(body: SkillWorkspaceSeedRequest): folder = os.path.join(SKILLS_WORKSPACE_DIR, body.workspace_id) @@ -141,10 +342,15 @@ async def get_skill(skill_id: str): @skills.router.post("/create") async def create_skill(body: SkillCreate): slug = body.name.lower().replace(" ", "-") - fpath = os.path.join(SKILLS_DIR, f"{slug}.md") + skill_dir = os.path.join(SKILLS_DIR, slug) + os.makedirs(skill_dir, exist_ok=True) + fpath = os.path.join(skill_dir, "SKILL.md") + final_content = _ensure_frontmatter( + body.content, name=body.name, description=body.description + ) with open(fpath, "w") as f: - f.write(body.content) + f.write(final_content) index = _load_index() index[slug] = { @@ -158,33 +364,65 @@ async def create_skill(body: SkillCreate): id=slug, name=body.name, description=body.description, - content=body.content, + content=final_content, file_path=fpath, command=body.command or slug, ) - pass + _invalidate_installed_slugs_cache() return {"ok": True, "skill": skill.model_dump()} @skills.router.put("/{skill_id}") async def update_skill(skill_id: str, body: SkillUpdate): - fpath = os.path.join(SKILLS_DIR, f"{skill_id}.md") + fpath = _skill_path(skill_id) if not os.path.exists(fpath): raise HTTPException(status_code=404, detail="Skill not found") - if body.content is not None: - with open(fpath, "w") as f: - f.write(body.content) - index = _load_index() meta = index.get(skill_id, {}) - if body.name is not None: - meta["name"] = body.name - if body.description is not None: - meta["description"] = body.description if body.command is not None: meta["command"] = body.command + + # Precedence for name/description, highest first: + # 1. explicit form field (`body.name` / `body.description`) + # 2. inline frontmatter inside `body.content` (so editing the YAML + # block in the editor textarea actually sticks instead of being + # silently clobbered by the stale index value) + # 3. previous index value + # 4. slug-derived fallback + # + inline = _parse_skill_frontmatter(body.content) if body.content else {} + if body.name is not None: + meta["name"] = body.name + elif inline.get("name"): + meta["name"] = inline["name"] + if body.description is not None: + meta["description"] = body.description + elif "description" in inline: + meta["description"] = inline["description"] index[skill_id] = meta + + if body.content is not None: + new_content = _ensure_frontmatter( + body.content, + name=meta.get("name", skill_id), + description=meta.get("description", ""), + ) + with open(fpath, "w") as f: + f.write(new_content) + elif body.name is not None or body.description is not None: + # Metadata changed but body wasn't supplied — patch the + # frontmatter on disk so the CLI listing stays in sync. + with open(fpath, encoding="utf-8") as f: + existing = f.read() + new_content = _ensure_frontmatter( + existing, + name=meta.get("name", skill_id), + description=meta.get("description", ""), + ) + with open(fpath, "w") as f: + f.write(new_content) + _save_index(index) with open(fpath) as f: @@ -203,10 +441,11 @@ async def update_skill(skill_id: str, body: SkillUpdate): @skills.router.delete("/{skill_id}") async def delete_skill(skill_id: str): - fpath = os.path.join(SKILLS_DIR, f"{skill_id}.md") - if os.path.exists(fpath): - os.remove(fpath) + skill_dir = os.path.join(SKILLS_DIR, skill_id) + if os.path.isdir(skill_dir): + shutil.rmtree(skill_dir) index = _load_index() index.pop(skill_id, None) _save_index(index) + _invalidate_installed_slugs_cache() return {"ok": True} diff --git a/backend/tests/test_agent_manager_unit.py b/backend/tests/test_agent_manager_unit.py index 0c0a702b..1ffaced6 100644 --- a/backend/tests/test_agent_manager_unit.py +++ b/backend/tests/test_agent_manager_unit.py @@ -314,25 +314,6 @@ def test_get_all_tool_names_drops_explicitly_denied_builtins(tmp_data_dirs): # --------------------------------------------------------------------------- -def test_ensure_cwd_git_repo_creates_repo_when_missing(tmp_path): - """Fresh tmp dir with no .git → function inits a repo + empty commit.""" - cwd = tmp_path / "fresh" - cwd.mkdir() - _ensure_cwd_git_repo(str(cwd), home=str(tmp_path)) - # Either .git lives here directly, or git decided we're already in - # a parent repo (the test runner's repo, for instance). Both are - # valid healthy outcomes. - if (cwd / ".git").exists(): - # Verify HEAD resolves — the function commits an empty seed. - import subprocess - head = subprocess.run( - ["git", "rev-parse", "--verify", "HEAD"], - cwd=str(cwd), - capture_output=True, - ) - assert head.returncode == 0 - - def test_ensure_cwd_git_repo_skips_risky_roots(tmp_path): """Calling on $HOME / / / parent-of-home must short-circuit and leave the directory untouched.""" @@ -416,24 +397,117 @@ def test_compose_system_prompt_all_none_returns_none(): assert AgentManager()._compose_system_prompt(None, None, None) is None -def test_run_agent_loop_disables_bundled_claude_code_skills(): - """Regression guard for `options_kwargs["skills"] = []` in _run_agent_loop. +def test_run_agent_loop_uses_skill_allowlist_not_kill_switch(): + """`_run_agent_loop` must hand the SDK a per-turn allowlist, not the + old `[]` kill-switch. - The claude-agent-sdk ships a Skill tool that surfaces Claude Code's - bundled plugin skills (/init, /review, /security-review, /simplify, - /loop, /schedule, /update-config, /keybindings-help, - /fewer-permission-prompts, /claude-api). These are inappropriate in - OpenSwarm — half mutate ~/.claude config the backend doesn't read, and - the rest reference slash commands the backend never intercepts. The - SDK's documented "skills off" signal is `skills=[]` on - ClaudeAgentOptions; user-attached skills bypass the Skill tool and - are unaffected. If this assignment ever gets dropped during a rebase - or refactor the bundled skills come back, so we pin it here. + The bundled Claude Code skills (/init, /review, /security-review, + /simplify, /loop, /schedule, /update-config, /keybindings-help, + /fewer-permission-prompts, /claude-api) need to stay suppressed — + half mutate ~/.claude state OpenSwarm doesn't read, the rest + reference slash commands the backend never intercepts. Passing + `None` to the SDK lets the CLI's defaults reintroduce them; passing + `[]` (the previous behavior) suppresses them but also kills user + skills. The right answer is `list[str]` with only user-installed + slugs, which is what `_resolve_sdk_skill_allowlist` produces. This + test pins the wiring so a future refactor can't silently revert. """ import inspect src = inspect.getsource(AgentManager._run_agent_loop) - assert 'options_kwargs["skills"] = []' in src + assert 'options_kwargs["skills"] = self._resolve_sdk_skill_allowlist(' in src + assert 'options_kwargs["skills"] = []' not in src + assert 'options_kwargs["skills"] = None' not in src + + +def test_resolve_sdk_skill_allowlist_empty_when_no_skills_installed( + monkeypatch, tmp_path +): + from backend.apps.skills import skills as skills_mod + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(tmp_path / "skills")) + assert AgentManager._resolve_sdk_skill_allowlist(None) == [] + assert AgentManager._resolve_sdk_skill_allowlist([]) == [] + + +def test_resolve_sdk_skill_allowlist_lists_only_directory_skills( + monkeypatch, tmp_path +): + """Only `/SKILL.md` entries count — flat files and dotfiles are + skipped since the CLI's discovery ignores them.""" + from backend.apps.skills import skills as skills_mod + + skills_root = tmp_path / "skills" + skills_root.mkdir() + (skills_root / "alpha").mkdir() + (skills_root / "alpha" / "SKILL.md").write_text("---\nname: alpha\n---\n") + (skills_root / "beta").mkdir() + (skills_root / "beta" / "SKILL.md").write_text("---\nname: beta\n---\n") + (skills_root / "no-skill-md").mkdir() + (skills_root / "no-skill-md" / "README.md").write_text("not a skill") + (skills_root / "stray.md").write_text("legacy flat file") + (skills_root / ".skills_index.json").write_text("{}") + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(skills_root)) + out = AgentManager._resolve_sdk_skill_allowlist(None) + assert out == ["alpha", "beta"], ( + "expected sorted slugs of directories containing SKILL.md, with " + "flat files / dotfiles / partial dirs excluded" + ) + + +def test_resolve_sdk_skill_allowlist_dedups_manually_attached( + monkeypatch, tmp_path +): + """Skills the user attached via `/` are eagerly injected into the + user message by `_resolve_attached_skills`; they must therefore drop + out of the SDK allowlist for this turn so the same content can't + re-enter via the lazy Skill-tool path.""" + from backend.apps.skills import skills as skills_mod + + skills_root = tmp_path / "skills" + skills_root.mkdir() + for slug in ("alpha", "beta", "gamma"): + (skills_root / slug).mkdir() + (skills_root / slug / "SKILL.md").write_text(f"---\nname: {slug}\n---\n") + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(skills_root)) + out = AgentManager._resolve_sdk_skill_allowlist( + [{"id": "beta", "name": "Beta"}] + ) + assert out == ["alpha", "gamma"] + + +def test_resolve_sdk_skill_allowlist_excludes_bundled_claude_code_names( + monkeypatch, tmp_path +): + """The allowlist must never contain bundled Claude Code skill names + just because someone happens to know them — only what's actually on + disk under SKILLS_DIR. This pins the behavior that the bundled + `/init`, `/review`, etc. stay invisible.""" + from backend.apps.skills import skills as skills_mod + + skills_root = tmp_path / "skills" + skills_root.mkdir() + (skills_root / "user-skill").mkdir() + (skills_root / "user-skill" / "SKILL.md").write_text("---\nname: x\n---\n") + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(skills_root)) + out = AgentManager._resolve_sdk_skill_allowlist(None) + bundled = { + "init", + "review", + "security-review", + "simplify", + "loop", + "schedule", + "update-config", + "keybindings-help", + "fewer-permission-prompts", + "claude-api", + } + assert not bundled.intersection(out) + assert "user-skill" in out def test_resolve_context_paths_empty_returns_empty(): diff --git a/backend/tests/test_api_skills.py b/backend/tests/test_api_skills.py index 9fa98b6d..e5512b93 100644 --- a/backend/tests/test_api_skills.py +++ b/backend/tests/test_api_skills.py @@ -1,13 +1,17 @@ """Smoke tests for /api/skills. -Skills are SKILL.md files synced into `~/.claude/skills`. Tests MUST -NEVER touch the user's real skills dir, so this whole module relies on -the `patched_skills_dir` fixture. +Skills live under `~/.claude/skills//SKILL.md` — the layout the +Claude Code CLI auto-discovers (verified against the bundled CLI's path +parser, which expects exactly four segments ending in `SKILL.md`). Tests +MUST NEVER touch the user's real skills dir, so this whole module relies +on the `patched_skills_dir` fixture. Tests: - empty list on a fresh dir - - dropping a SKILL.md into the patched dir surfaces in /list + - dropping a `/SKILL.md` into the patched dir surfaces in /list - create / get / update / delete round-trip + - frontmatter is stamped on create even when the body has none + - flat-file legacy layout migrates into the directory layout on startup - workspace/seed writes SKILL.md and meta.json """ @@ -24,10 +28,12 @@ def test_list_empty_on_fresh_dir(client, patched_skills_dir): def test_list_picks_up_dropped_skill(client, patched_skills_dir): - """Filesystem-driven discovery — dropping a .md file is enough.""" - skill_path = os.path.join(patched_skills_dir, "test-skill.md") + """Filesystem-driven discovery — dropping a `/SKILL.md` is enough.""" + skill_dir = os.path.join(patched_skills_dir, "test-skill") + os.makedirs(skill_dir) + skill_path = os.path.join(skill_dir, "SKILL.md") with open(skill_path, "w") as f: - f.write("# Test Skill\n\nBody content.\n") + f.write("---\nname: Test Skill\ndescription: a test\n---\n\nBody.\n") resp = client.get("/api/skills/list") assert resp.status_code == 200 @@ -35,6 +41,18 @@ def test_list_picks_up_dropped_skill(client, patched_skills_dir): assert any(s["id"] == "test-skill" for s in skills) +def test_list_ignores_legacy_flat_files(client, patched_skills_dir): + """A stray `.md` at the top level isn't a discoverable skill — + the CLI requires the directory layout, so we mirror that.""" + flat = os.path.join(patched_skills_dir, "stray.md") + with open(flat, "w") as f: + f.write("body without a home") + + resp = client.get("/api/skills/list") + assert resp.status_code == 200 + assert resp.json() == {"skills": []} + + def test_create_get_update_delete_skill(client, patched_skills_dir): create = client.post( "/api/skills/create", @@ -47,6 +65,9 @@ def test_create_get_update_delete_skill(client, patched_skills_dir): assert create.status_code == 200, create.text skill_id = create.json()["skill"]["id"] + skill_md = os.path.join(patched_skills_dir, skill_id, "SKILL.md") + assert os.path.isfile(skill_md), f"expected SKILL.md at {skill_md}" + fetched = client.get(f"/api/skills/{skill_id}") assert fetched.status_code == 200 assert fetched.json()["name"] == "Smoke" @@ -58,9 +79,296 @@ def test_create_get_update_delete_skill(client, patched_skills_dir): assert update.status_code == 200 assert update.json()["skill"]["description"] == "smoke v2" + with open(skill_md, encoding="utf-8") as f: + on_disk = f.read() + assert "description: smoke v2" in on_disk, ( + "metadata-only update should patch SKILL.md frontmatter so the CLI " + "listing stays in sync" + ) + deleted = client.delete(f"/api/skills/{skill_id}") assert deleted.status_code == 200 - assert not os.path.exists(os.path.join(patched_skills_dir, f"{skill_id}.md")) + assert not os.path.exists(os.path.join(patched_skills_dir, skill_id)) + + +def test_create_stamps_frontmatter_when_body_has_none(client, patched_skills_dir): + """Without frontmatter the CLI silently drops a skill from the listing, + so the create endpoint MUST emit a YAML block even if the caller sent + a plain Markdown body.""" + create = client.post( + "/api/skills/create", + json={ + "name": "Bare", + "description": "no fm in body", + "content": "Just a body, no frontmatter.\n", + }, + ) + assert create.status_code == 200, create.text + skill_id = create.json()["skill"]["id"] + skill_md = os.path.join(patched_skills_dir, skill_id, "SKILL.md") + + with open(skill_md, encoding="utf-8") as f: + on_disk = f.read() + assert on_disk.startswith("---\n") + assert "name: Bare" in on_disk + assert "description: no fm in body" in on_disk + assert "Just a body, no frontmatter." in on_disk + + +def test_inline_frontmatter_edits_in_body_are_honored(client, patched_skills_dir): + """Editing the `---` block inside the content textarea must stick when + the caller doesn't separately override via the form fields. + + Previously `update_skill` always stamped the file with `meta`'s + pre-existing name/description, silently discarding the user's inline + YAML edits. New precedence: explicit form field > inline frontmatter + > previous index value. This test exercises the inline-only path. + """ + create = client.post( + "/api/skills/create", + json={"name": "Original", "description": "old desc", "content": "body"}, + ) + assert create.status_code == 200, create.text + skill_id = create.json()["skill"]["id"] + + new_body = ( + "---\n" + "name: EditedInline\n" + "description: edited inline\n" + "---\n\n" + "fresh body\n" + ) + resp = client.put(f"/api/skills/{skill_id}", json={"content": new_body}) + assert resp.status_code == 200, resp.text + + listed = client.get("/api/skills/list").json()["skills"] + surfaced = next(s for s in listed if s["id"] == skill_id) + assert surfaced["name"] == "EditedInline", ( + "inline frontmatter `name:` must beat stale index value when the " + "PUT body doesn't include an explicit `name` field" + ) + assert surfaced["description"] == "edited inline" + + +def test_form_name_still_beats_inline_frontmatter(client, patched_skills_dir): + """The opposite direction of the precedence rule: when the caller + sends BOTH an explicit `name` form field AND inline frontmatter, the + form field wins. This is the typical frontend save flow.""" + create = client.post( + "/api/skills/create", + json={"name": "Original", "description": "old", "content": "body"}, + ) + skill_id = create.json()["skill"]["id"] + + conflicting_body = ( + "---\n" + "name: FromInline\n" + "description: from inline\n" + "---\n\n" + "body\n" + ) + resp = client.put( + f"/api/skills/{skill_id}", + json={"name": "FromForm", "content": conflicting_body}, + ) + assert resp.status_code == 200 + + listed = client.get("/api/skills/list").json()["skills"] + surfaced = next(s for s in listed if s["id"] == skill_id) + assert surfaced["name"] == "FromForm", ( + "explicit form `name` must beat inline frontmatter when both " + "are supplied" + ) + # description wasn't on the form, so inline wins + assert surfaced["description"] == "from inline" + + +def test_frontmatter_roundtrips_special_characters(client, patched_skills_dir): + """A description with embedded quotes / colons / hashes must survive + arbitrary save-load-save cycles unchanged. The previous parser blindly + stripped one pair of outer quotes without un-escaping `\\"`, so each + save accumulated more backslashes until the description was a wall of + `\\\\\\\\\\"`. Regression test for that bug. + """ + tricky = 'has "quotes": and # hash' + create = client.post( + "/api/skills/create", + json={"name": "Tricky", "description": tricky, "content": "body\n"}, + ) + assert create.status_code == 200, create.text + skill_id = create.json()["skill"]["id"] + + # Three metadata-only saves: every one re-stamps frontmatter, so a + # lossy decoder would compound the damage. The list endpoint reads + # the description back from disk via the frontmatter parser. + for _ in range(3): + resp = client.put(f"/api/skills/{skill_id}", json={"description": tricky}) + assert resp.status_code == 200 + + listed = client.get("/api/skills/list").json()["skills"] + surfaced = next(s for s in listed if s["id"] == skill_id) + assert surfaced["description"] == tricky, ( + "description with quotes/colons/hash must round-trip exactly across " + "successive saves; lossy parser would accumulate backslashes" + ) + + +def test_migration_moves_flat_file_into_directory(patched_skills_dir, monkeypatch): + """Pre-existing `.md` files (from before the directory-layout + migration) must be relocated to `/SKILL.md` exactly once at + startup so the CLI's discovery picks them up.""" + from backend.apps.skills import skills as skills_mod + + flat = os.path.join(patched_skills_dir, "legacy.md") + with open(flat, "w") as f: + f.write("# Legacy\n\nbody.") + skills_mod._save_index({"legacy": {"name": "Legacy", "description": "old"}}) + + skills_mod._migrate_flat_to_dir_layout() + + assert not os.path.exists(flat), "flat file should be removed after migration" + new_path = os.path.join(patched_skills_dir, "legacy", "SKILL.md") + assert os.path.isfile(new_path), f"expected migrated SKILL.md at {new_path}" + with open(new_path, encoding="utf-8") as f: + migrated = f.read() + assert migrated.startswith("---\n"), "migration should stamp frontmatter" + assert "name: Legacy" in migrated + assert "description: old" in migrated + assert "body." in migrated + + # Idempotent: a second run must not crash or duplicate work. + skills_mod._migrate_flat_to_dir_layout() + assert os.path.isfile(new_path) + + +def test_migration_preserves_existing_directory(patched_skills_dir): + """If a directory already exists for a slug, the flat file is removed + (the dir wins) — never overwritten.""" + from backend.apps.skills import skills as skills_mod + + skill_dir = os.path.join(patched_skills_dir, "shared") + os.makedirs(skill_dir) + canonical = os.path.join(skill_dir, "SKILL.md") + with open(canonical, "w") as f: + f.write("---\nname: shared\n---\n\ncanonical body") + flat = os.path.join(patched_skills_dir, "shared.md") + with open(flat, "w") as f: + f.write("legacy body that should not win") + + skills_mod._migrate_flat_to_dir_layout() + + assert not os.path.exists(flat) + with open(canonical, encoding="utf-8") as f: + assert "canonical body" in f.read() + + +def test_installed_slugs_cache_avoids_rescanning(patched_skills_dir, monkeypatch): + """`get_installed_slugs` must hit the filesystem on the first call, + then serve subsequent calls from cache without re-walking SKILLS_DIR. + The agent loop calls this on every turn, so a per-turn rescan would + cost N*T syscalls in the hot path for N skills * T turns.""" + from backend.apps.skills import skills as skills_mod + + skill_dir = os.path.join(patched_skills_dir, "alpha") + os.makedirs(skill_dir) + with open(os.path.join(skill_dir, "SKILL.md"), "w") as f: + f.write("---\nname: alpha\n---\n") + + real_listdir = os.listdir + call_count = {"n": 0} + + def counting_listdir(path): + if os.path.abspath(path) == os.path.abspath(patched_skills_dir): + call_count["n"] += 1 + return real_listdir(path) + + monkeypatch.setattr(skills_mod.os, "listdir", counting_listdir) + skills_mod._invalidate_installed_slugs_cache() + + assert skills_mod.get_installed_slugs() == ["alpha"] + first = call_count["n"] + assert first == 1, "first call must rescan" + + for _ in range(5): + assert skills_mod.get_installed_slugs() == ["alpha"] + assert call_count["n"] == first, ( + "subsequent calls must be served from cache without re-listing " + "SKILLS_DIR" + ) + + +def test_installed_slugs_cache_invalidated_on_create(client, patched_skills_dir): + """Creating a skill via /api/skills/create must invalidate the cache + so the next allowlist query reflects the new slug. Without this the + next agent turn would silently fail to expose the freshly-installed + skill to the model.""" + from backend.apps.skills import skills as skills_mod + + assert skills_mod.get_installed_slugs() == [] + + resp = client.post( + "/api/skills/create", + json={ + "name": "FreshSkill", + "description": "test", + "content": "body", + }, + ) + assert resp.status_code == 200 + skill_id = resp.json()["skill"]["id"] + + assert skill_id in skills_mod.get_installed_slugs(), ( + "newly-created slug should appear without an explicit " + "cache-bust call from the test" + ) + + +def test_installed_slugs_cache_invalidated_on_delete(client, patched_skills_dir): + """Deleting a skill must invalidate the cache so the slug drops out + of the next allowlist query. Otherwise the agent would keep being + told about a skill the user just removed, and a `Skill(slug)` call + would fail in the CLI with a confusing 'skill not found' error.""" + from backend.apps.skills import skills as skills_mod + + client.post( + "/api/skills/create", + json={"name": "Doomed", "description": "x", "content": "body"}, + ) + assert "doomed" in skills_mod.get_installed_slugs() + + resp = client.delete("/api/skills/doomed") + assert resp.status_code == 200 + assert "doomed" not in skills_mod.get_installed_slugs(), ( + "deleted slug must drop out of the cached list immediately" + ) + + +def test_installed_slugs_cache_keyed_by_skills_dir(monkeypatch, tmp_path): + """The cache is keyed by SKILLS_DIR so a test that monkeypatches + that dir gets a fresh scan automatically — important because pytest + re-uses the same module-scoped cache across all tests, and we don't + want test A's skills bleeding into test B's view.""" + from backend.apps.skills import skills as skills_mod + + dir_a = tmp_path / "a" + dir_a.mkdir() + (dir_a / "only-in-a").mkdir() + (dir_a / "only-in-a" / "SKILL.md").write_text("---\nname: a\n---\n") + + dir_b = tmp_path / "b" + dir_b.mkdir() + (dir_b / "only-in-b").mkdir() + (dir_b / "only-in-b" / "SKILL.md").write_text("---\nname: b\n---\n") + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(dir_a)) + skills_mod._invalidate_installed_slugs_cache() + assert skills_mod.get_installed_slugs() == ["only-in-a"] + + monkeypatch.setattr(skills_mod, "SKILLS_DIR", str(dir_b)) + assert skills_mod.get_installed_slugs() == ["only-in-b"], ( + "switching SKILLS_DIR must implicitly invalidate the cache " + "without an explicit bust" + ) def test_workspace_seed_writes_files(client, patched_skills_dir):