fix(skills): markdown save must not rename the skill, so delete keeps working (#1333) (#1365)

POST /api/skills/{id}/markdown set sk.name = slugify(sk.name or match['name']),
taking the name parsed from the edited markdown frontmatter. A changed name
makes update_skill() move the skill directory on disk and re-key its usage
sidecar, orphaning the original id. The UI still holds that original id, so the
next DELETE /api/skills/{id} fails the name/id lookup and 404s — 'can't delete
them now'.

The audit save path (_apply_skill_md) already guards against exactly this with
sk.name = name and an explicit 'must NEVER rename the skill' comment. Apply the
same pin here: keep the stored name on markdown save (content edits still take
effect; only the rename is suppressed). Drops the now-unused slugify import.

Adds tests/test_skill_save_no_rename.py: saving markdown whose frontmatter
renames the skill keeps the original name and applies the edit, and a
subsequent delete-by-original-id succeeds. Pure unit test — calls the route
handlers directly with a mock Request (no server/network), like
test_skills_delete_owner.py.

Co-authored-by: lalalune <shawgotbags@gmail.com>

routes/skills_routes.py

@@ -1442,7 +1442,7 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter:
     @router.post("/{skill_id}/markdown")
     async def save_skill_markdown(request: Request, skill_id: str):
         """Replace SKILL.md with new raw content. Parses + validates first."""
-        from services.memory.skill_format import Skill, slugify
+        from services.memory.skill_format import Skill
         user = _owner(request)
         body = await request.json()
         new_content = body.get("markdown")
@@ -1457,7 +1457,10 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter:
             sk = Skill.from_markdown(new_content)
         except Exception as e:
             raise HTTPException(400, f"Could not parse SKILL.md: {e}")
-        sk.name = slugify(sk.name or match.get("name"))
+        # Never rename on save: a changed `name` in the markdown would move
+        # the skill dir (update_skill) and orphan the original id, so a later
+        # delete 404s (#1333). Pin to the stored name, like _apply_skill_md.
+        sk.name = match.get("name")
         if not sk.owner:
             sk.owner = match.get("owner") or user
         ok = skills_manager.update_skill(match.get("name"), {

tests/test_skill_save_no_rename.py

@@ -0,0 +1,120 @@
+"""Saving a skill's markdown must NOT rename it (issue #1333: can't delete skills).
+
+`save_skill_markdown` (POST /api/skills/{id}/markdown) parsed the new markdown
+and set `sk.name = slugify(sk.name or match["name"])` — so editing the frontmatter
+`name:` silently renamed the skill, which moves its directory on disk
+(`update_skill`) and orphans the original id. A later DELETE by the id the UI
+still holds then 404s ("can't delete them now"). The audit save path
+(`_apply_skill_md`) already pins the name with the comment that a save must
+NEVER rename; this locks that same guarantee for the markdown-save endpoint.
+
+Pure unit test: calls the route handlers directly with a mock Request (no
+server, network, or browser), mirroring tests/test_skills_delete_owner.py.
+"""
+
+import json
+import textwrap
+from pathlib import Path
+
+import pytest
+from fastapi import Request
+from fastapi.datastructures import State
+
+from services.memory.skills import SkillsManager
+from services.memory.skill_format import slugify
+from routes.skills_routes import setup_skills_routes
+
+
+def _write_skill_md(skills_root: Path, category: str, name: str, owner: str) -> Path:
+    skill_dir = skills_root / slugify(category or "general", fallback="general") / name
+    skill_dir.mkdir(parents=True, exist_ok=True)
+    md = textwrap.dedent(f"""\
+        ---
+        name: {name}
+        description: original description
+        version: 1.0.0
+        category: {category}
+        tags: []
+        status: draft
+        confidence: 0.8
+        source: learned
+        owner: {owner}
+        created: 2026-01-01T00:00:00Z
+        ---
+
+        # When to use
+        test
+
+        # Procedure
+        - step 1
+        """)
+    path = skill_dir / "SKILL.md"
+    path.write_text(md, encoding="utf-8")
+    return path
+
+
+def _md_named(name: str) -> str:
+    return textwrap.dedent(f"""\
+        ---
+        name: {name}
+        description: edited description
+        version: 1.0.0
+        category: general
+        tags: []
+        status: draft
+        confidence: 0.8
+        source: learned
+        owner: alice
+        created: 2026-01-01T00:00:00Z
+        ---
+
+        # When to use
+        edited
+
+        # Procedure
+        - step 1
+        """)
+
+
+def _request(user: str, body: dict | None = None) -> Request:
+    scope = {"type": "http", "app": type("App", (), {"state": State()})(),
+             "state": {"current_user": user}, "headers": []}
+    if body is None:
+        return Request(scope=scope)
+
+    async def _receive():
+        return {"type": "http.request", "body": json.dumps(body).encode(), "more_body": False}
+
+    return Request(scope=scope, receive=_receive)
+
+
+def _handler(router, path: str, method: str):
+    return next(r.endpoint for r in router.routes
+               if r.path == path and method in r.methods)
+
+
+@pytest.mark.asyncio
+async def test_markdown_save_does_not_rename_then_delete_works(tmp_path):
+    skills_root = tmp_path / "skills"
+    skills_root.mkdir(parents=True, exist_ok=True)
+    _write_skill_md(skills_root, "general", "test-skill", "alice")
+
+    sm = SkillsManager(str(tmp_path))
+    router = setup_skills_routes(sm)
+    save = _handler(router, "/api/skills/{skill_id}/markdown", "POST")
+    delete = _handler(router, "/api/skills/{skill_id}", "DELETE")
+
+    # Save markdown whose frontmatter renames the skill. The save must keep the
+    # original name (no rename), so the returned name is unchanged.
+    res = await save(_request("alice", {"markdown": _md_named("renamed-skill")}), "test-skill")
+    assert res["name"] == "test-skill", f"save renamed the skill to {res.get('name')!r}"
+
+    # The skill still lives under its original id (the edit DID apply).
+    names = {s.get("name") for s in sm.load(owner="alice")}
+    assert names == {"test-skill"}, names
+    descriptions = {s.get("description") for s in sm.load(owner="alice")}
+    assert "edited description" in descriptions  # the content edit took effect
+
+    # The crux of #1333: deleting by the original id now succeeds.
+    assert await delete(_request("alice"), "test-skill") == {"ok": True}
+    assert sm.load(owner="alice") == []