Skills: delete owner-scoped skills with owner
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.
This commit is contained in:
@@ -1511,7 +1511,7 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter:
|
|||||||
if not match:
|
if not match:
|
||||||
raise HTTPException(404, "Skill not found")
|
raise HTTPException(404, "Skill not found")
|
||||||
_verify_owner(match, user)
|
_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:
|
if not ok:
|
||||||
raise HTTPException(404, "Skill not found")
|
raise HTTPException(404, "Skill not found")
|
||||||
return {"ok": True}
|
return {"ok": True}
|
||||||
|
|||||||
106
tests/test_skills_delete_owner.py
Normal file
106
tests/test_skills_delete_owner.py
Normal file
@@ -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()
|
||||||
Reference in New Issue
Block a user