From 9835c1a7f98266a4df941b2bc0853a5ab0a0fbc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Vind-Gardo=C8=99?= <41092915+robyroro@users.noreply.github.com> Date: Sat, 31 Oct 2026 15:34:05 +0200 Subject: [PATCH] fix(skills): refuse renaming a skill onto a name the owner already uses (#6941) update_skill only checked whether the target directory existed. A skill with that name in another category lives elsewhere on disk, so the rename went through and left two skills with one name. Update and delete look skills up by name, so the next delete removed whichever file os.walk reached first. Check the owner's skills in every category before renaming, and answer 409 from PUT /api/skills/{id} instead of the misleading 404. --- routes/skills_routes.py | 5 ++ services/memory/skills.py | 16 ++++++ tests/test_skill_rename_collision.py | 83 ++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 tests/test_skill_rename_collision.py 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")]