diff --git a/routes/skills_routes.py b/routes/skills_routes.py index f469fc305..3dd35d93b 100644 --- a/routes/skills_routes.py +++ b/routes/skills_routes.py @@ -15,6 +15,7 @@ import httpx from fastapi import APIRouter, HTTPException, Request from pydantic import BaseModel, Field +from services.memory.skill_format import slugify from services.memory.skills import SkillsManager from src.auth_helpers import get_current_user from services.skills import evaluation as _evaluation @@ -836,6 +837,10 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter: updates = body.dict(exclude_none=True) if not updates: return {"ok": True} + if "name" in updates: + new_name = slugify(updates["name"] or match.get("name")) + if new_name != match.get("name") and skills_manager.has_skill(new_name, owner=user): + raise HTTPException(409, f"A skill named '{new_name}' already exists") ok = skills_manager.update_skill(match.get("name"), updates, owner=user) if not ok: raise HTTPException(404, "Skill not found") diff --git a/services/memory/skills.py b/services/memory/skills.py index 2d0ad7063..f037a9383 100644 --- a/services/memory/skills.py +++ b/services/memory/skills.py @@ -500,6 +500,19 @@ class SkillsManager: sk.path = self._skill_file(cat, nm) return sk.to_dict() + def has_skill(self, name: str, owner: Optional[str] = None) -> bool: + """Whether `owner` already has a skill called `name`, in any category. + + Update and delete find a skill by name within its owner, so two of + one owner's skills sharing a name would make them act on whichever + file is walked first. + """ + for path in self._iter_skill_files(): + sk = self._read_skill(path) + if sk and sk.name == name and (sk.owner or "") == (owner or ""): + return True + return False + def update_skill(self, skill_id: str, updates: Dict, owner: Optional[str] = None) -> bool: """`skill_id` is the slug name. Allows updating any field plus renames if `name` changes (file is moved on disk). @@ -551,6 +564,9 @@ class SkillsManager: # Rename new_name = slugify(updates.get("name") or sk.name) if new_name != sk.name: + if self.has_skill(new_name, owner=owner): + logger.warning(f"Skill rename target name in use: {new_name}") + return False sk.name = new_name # Write to potentially new path diff --git a/tests/test_skill_rename_collision.py b/tests/test_skill_rename_collision.py new file mode 100644 index 000000000..b4a2c58fa --- /dev/null +++ b/tests/test_skill_rename_collision.py @@ -0,0 +1,83 @@ +"""Renaming a skill onto a name the owner already uses must be refused. + +``update_skill`` only checked whether the target directory existed. A +skill in another category with the wanted name lives in a different +directory, so the rename went through and the owner ended up with two +skills sharing one name. Update and delete look skills up by name, so the +next DELETE removed whichever of the two ``os.walk`` reached first, which +was often the one the user did not pick. +""" +import pytest + +from routes.skills_routes import SkillUpdateRequest, setup_skills_routes +from services.memory.skills import SkillsManager + +from tests.test_skills_routes_owner_update import _request, _route_handler, _write_skill_md + + +def _two_skills(tmp_path, owner="alice"): + mgr = SkillsManager(str(tmp_path)) + mgr.add_skill(name="deploy-app", description="Deploy the web app", category="ops", + source="user", procedure=["ship it"], owner=owner) + mgr.add_skill(name="draft-notes", description="Draft meeting notes", category="writing", + source="user", procedure=["write"], owner=owner) + return mgr + + +def _names(mgr, owner="alice"): + return sorted((s["name"], s["category"]) for s in mgr.load(owner=owner)) + + +def test_rename_onto_a_name_in_another_category_is_refused(tmp_path): + mgr = _two_skills(tmp_path) + + assert mgr.update_skill("draft-notes", {"name": "deploy-app"}, owner="alice") is False + assert _names(mgr) == [("deploy-app", "ops"), ("draft-notes", "writing")] + + +def test_delete_after_refused_rename_removes_only_the_named_skill(tmp_path): + mgr = _two_skills(tmp_path) + mgr.update_skill("draft-notes", {"name": "deploy-app"}, owner="alice") + + assert mgr.delete_skill("draft-notes", owner="alice") is True + assert _names(mgr) == [("deploy-app", "ops")] + + +def test_rename_to_a_free_name_still_moves_the_skill(tmp_path): + mgr = _two_skills(tmp_path) + + assert mgr.update_skill("draft-notes", {"name": "meeting-notes"}, owner="alice") is True + assert _names(mgr) == [("deploy-app", "ops"), ("meeting-notes", "writing")] + + +def test_another_owners_skill_does_not_block_the_rename(tmp_path): + skills_root = tmp_path / "skills" + _write_skill_md(skills_root, "bob-cat", "caveman-mode", "bob") + _write_skill_md(skills_root, "alice-cat", "terse-mode", "alice") + mgr = SkillsManager(str(tmp_path)) + + assert mgr.update_skill("terse-mode", {"name": "caveman-mode"}, owner="alice") is True + assert [s["name"] for s in mgr.load(owner="alice")] == ["caveman-mode"] + assert [s["name"] for s in mgr.load(owner="bob")] == ["caveman-mode"] + + +def test_keeping_the_same_name_is_not_a_conflict(tmp_path): + mgr = _two_skills(tmp_path) + + assert mgr.update_skill("draft-notes", {"name": "Draft Notes", "status": "published"}, + owner="alice") is True + assert [s["status"] for s in mgr.load(owner="alice") if s["name"] == "draft-notes"] == ["published"] + + +@pytest.mark.asyncio +async def test_put_route_answers_409_for_a_taken_name(tmp_path): + from fastapi import HTTPException + + mgr = _two_skills(tmp_path) + update_route = _route_handler(setup_skills_routes(mgr), "/api/skills/{skill_id}", "PUT") + + with pytest.raises(HTTPException) as exc: + await update_route(_request("alice"), "draft-notes", SkillUpdateRequest(name="deploy-app")) + + assert exc.value.status_code == 409 + assert _names(mgr) == [("deploy-app", "ops"), ("draft-notes", "writing")]