From cd247ed107886ff1f3cbf2a0825f1a0b2a087f60 Mon Sep 17 00:00:00 2001
From: Tatlatat <hungdotmn@gmail.com>
Date: Tue, 2 Jun 2026 18:28:36 +0700
Subject: [PATCH] Skills: delete owner-scoped skills with owner
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The DELETE /api/skills/{skill_id} handler resolves the caller, loads the
skill with skills_manager.load(owner=user), and verifies ownership with
_verify_owner(match, user) — but then calls
skills_manager.delete_skill(match.get("name")) without the owner.

SkillsManager.delete_skill filters candidates with
`(sk.owner or "") != (owner or "")`, so when owner is None an owner-scoped
skill is skipped and the method returns False. The route then raises a
spurious 404 "Skill not found" — meaning a logged-in user can never delete
their own skills through the API.

Pass the resolved owner through to delete_skill so the skill is matched and
removed.

tests/test_skills_delete_owner.py drops a real owner-scoped SKILL.md on disk
and (1) checks the manager directly: delete_skill without owner returns
False (regression lock) while delete_skill(owner="alice") returns True and
removes the dir; (2) drives the real DELETE route handler and asserts it
returns {"ok": True} and deletes the file. The route test fails before this
change (404). Real SkillsManager + real filesystem, no mocking.
---
 routes/skills_routes.py           |   2 +-
 tests/test_skills_delete_owner.py | 106 ++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 tests/test_skills_delete_owner.py

diff --git a/routes/skills_routes.py b/routes/skills_routes.py
index a9dc982..3566a63 100644
--- a/routes/skills_routes.py
+++ b/routes/skills_routes.py
@@ -1511,7 +1511,7 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter:
         if not match:
             raise HTTPException(404, "Skill not found")
         _verify_owner(match, user)
-        ok = skills_manager.delete_skill(match.get("name"))
+        ok = skills_manager.delete_skill(match.get("name"), owner=user)
         if not ok:
             raise HTTPException(404, "Skill not found")
         return {"ok": True}
diff --git a/tests/test_skills_delete_owner.py b/tests/test_skills_delete_owner.py
new file mode 100644
index 0000000..493992a
--- /dev/null
+++ b/tests/test_skills_delete_owner.py
@@ -0,0 +1,106 @@
+import os
+import pytest
+import textwrap
+from pathlib import Path
+from fastapi import Request, HTTPException
+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, description: str) -> Path:
+    """Drop a real SKILL.md on disk for the given owner."""
+    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: {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 test_delete_skill_manager_direct_scoping(tmp_path):
+    skills_root = tmp_path / "skills"
+    skills_root.mkdir(parents=True, exist_ok=True)
+    
+    # Create an owner-scoped skill (owner="alice")
+    path = _write_skill_md(
+        skills_root,
+        category="general",
+        name="test-skill",
+        owner="alice",
+        description="test",
+    )
+    
+    sm = SkillsManager(str(tmp_path))
+    
+    # 1. Assert that calling delete_skill without owner returns False (documents the bug/regression lock)
+    assert sm.delete_skill("test-skill") is False
+    assert path.exists() is True
+    
+    # 2. Call the manager exactly as the fixed route does (with owner), assert it returns True and the skill is gone
+    assert sm.delete_skill("test-skill", owner="alice") is True
+    assert path.exists() is False
+
+
+@pytest.mark.asyncio
+async def test_delete_skill_route_handler_scoping(tmp_path):
+    skills_root = tmp_path / "skills"
+    skills_root.mkdir(parents=True, exist_ok=True)
+    
+    # Create an owner-scoped skill (owner="alice")
+    path = _write_skill_md(
+        skills_root,
+        category="general",
+        name="test-skill",
+        owner="alice",
+        description="test",
+    )
+    
+    sm = SkillsManager(str(tmp_path))
+    router = setup_skills_routes(sm)
+    
+    # Find the delete route handler endpoint
+    delete_route_handler = next(
+        route.endpoint for route in router.routes
+        if route.path == "/api/skills/{skill_id}" and "DELETE" in route.methods
+    )
+    
+    # Construct a mock FastAPI Request
+    class DummyApp:
+        state = State()
+    app = DummyApp()
+    
+    request = Request(scope={
+        "type": "http",
+        "app": app,
+        "state": {
+            "current_user": "alice"
+        }
+    })
+    
+    # Before the fix, this raises HTTPException 404 because delete_skill was called without owner.
+    # After the fix, it deletes successfully and returns {"ok": True}.
+    res = await delete_route_handler(request, "test-skill")
+    assert res == {"ok": True}
+    assert not path.exists()