mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-25 21:34:47 -06:00
Patrick Buckley
15f7c7499c
The skills.sh install path was failing with 404s because their public
API surface changed: /api/skills/{id} is gone, replaced by
/api/skill/[owner]/[repo]/[skill] (auth-walled) and
/api/download/[owner]/[repo]/[skill] (unauthenticated, returns the
SKILL.md + bundled resources inline as JSON). The error was not
surfacing in logs because admin_skill_install had a silent
`except Exception:` around create_prompt_template that relabeled every
storage failure as "conflict" with no log entry.
- Replace SkillsShClient.resolve_github_url with download_skill that
hits /api/download/{owner}/{repo}/{skill} and returns a SkillPackage
directly. No GitHub round-trip; no rate-limit surface.
- Add _split_skills_sh_id with strict per-segment charset validation
([A-Za-z0-9._-]+) so URL-hostile content can't produce a malformed
request or divergent persisted source_url.
- Use len(contents) instead of len(contents.encode("utf-8",
errors="ignore")) for the SKILL.md size cap — errors='ignore' was
silently dropping invalid units, making the cap bypassable.
- Extract _accept_resource(rel_path, byte_size) gate predicate; share
it between download_skill and the GitHub _find_resource_files helper.
- Have search() derive a deterministic source_url from the skill id
when /api/search omits one (which it currently always does), so the
discover-UI "already installed" check matches what download_skill
persists.
- Add structured logging across admin_skill_install and
admin_skill_discover: a shared _log_install_failure helper for the
four except branches (was four near-duplicate log calls with one
drift), plus per-resource failure tallying — partial-resource
installs now surface failed_resources in the response and audit
record instead of silently committing the skill row with missing
assets.
Tests: 7 new — empty/non-list files, oversized SKILL.md, resource
cap, non-text extension filtering, plus _split_skills_sh_id charset
rejection (whitespace, query chars). Verified end-to-end against
live skills.sh with tavily-search.
406 lines
14 KiB
Python
406 lines
14 KiB
Python
"""Tests for skill discovery admin API endpoints."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from typing import TYPE_CHECKING, Any
|
|
from unittest.mock import AsyncMock, patch
|
|
|
|
import pytest
|
|
from starlette.applications import Starlette
|
|
from starlette.middleware import Middleware
|
|
from starlette.middleware.base import BaseHTTPMiddleware
|
|
from starlette.routing import Mount, Route
|
|
from starlette.testclient import TestClient
|
|
|
|
if TYPE_CHECKING:
|
|
from starlette.requests import Request
|
|
from starlette.responses import Response
|
|
|
|
from turnstone.console.server import admin_skill_discover, admin_skill_install
|
|
from turnstone.core.auth import AuthResult
|
|
from turnstone.core.skill_parser import ParsedSkill
|
|
from turnstone.core.skill_sources import (
|
|
SkillListing,
|
|
SkillNotFoundError,
|
|
SkillPackage,
|
|
SkillSourceError,
|
|
)
|
|
from turnstone.core.storage._sqlite import SQLiteBackend
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Auth middleware
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class _InjectAuthMiddleware(BaseHTTPMiddleware):
|
|
"""Inject an admin auth result with admin.skills permission."""
|
|
|
|
async def dispatch(self, request: Request, call_next: Any) -> Response:
|
|
request.state.auth_result = AuthResult(
|
|
user_id="test-user",
|
|
scopes=frozenset({"approve"}),
|
|
token_source="config",
|
|
permissions=frozenset({"read", "write", "approve", "admin.skills"}),
|
|
)
|
|
return await call_next(request)
|
|
|
|
|
|
class _InjectAuthNoSkillsMiddleware(BaseHTTPMiddleware):
|
|
"""Inject an auth result WITHOUT admin.skills permission."""
|
|
|
|
async def dispatch(self, request: Request, call_next: Any) -> Response:
|
|
request.state.auth_result = AuthResult(
|
|
user_id="test-user",
|
|
scopes=frozenset({"approve"}),
|
|
token_source="jwt",
|
|
permissions=frozenset({"read", "write", "approve"}),
|
|
)
|
|
return await call_next(request)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Fixtures
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_ROUTES = [
|
|
Mount(
|
|
"/v1",
|
|
routes=[
|
|
Route("/api/admin/skills/discover", admin_skill_discover),
|
|
Route(
|
|
"/api/admin/skills/install",
|
|
admin_skill_install,
|
|
methods=["POST"],
|
|
),
|
|
],
|
|
),
|
|
]
|
|
|
|
|
|
@pytest.fixture
|
|
def storage(tmp_path):
|
|
return SQLiteBackend(str(tmp_path / "test.db"))
|
|
|
|
|
|
@pytest.fixture
|
|
def client(storage):
|
|
app = Starlette(
|
|
routes=_ROUTES,
|
|
middleware=[Middleware(_InjectAuthMiddleware)],
|
|
)
|
|
app.state.auth_storage = storage
|
|
return TestClient(app)
|
|
|
|
|
|
@pytest.fixture
|
|
def client_no_perm(storage):
|
|
app = Starlette(
|
|
routes=_ROUTES,
|
|
middleware=[Middleware(_InjectAuthNoSkillsMiddleware)],
|
|
)
|
|
app.state.auth_storage = storage
|
|
return TestClient(app)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helpers
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _sample_listing(
|
|
name: str = "test-skill",
|
|
skill_id: str = "owner/repo/test-skill",
|
|
) -> SkillListing:
|
|
return SkillListing(
|
|
id=skill_id,
|
|
name=name,
|
|
description="A test skill",
|
|
author="Test Author",
|
|
source="skills.sh",
|
|
source_url="https://github.com/owner/repo",
|
|
install_count=42,
|
|
tags=["test"],
|
|
)
|
|
|
|
|
|
def _sample_package(
|
|
name: str = "test-skill",
|
|
source_url: str = "https://github.com/owner/repo",
|
|
) -> SkillPackage:
|
|
return SkillPackage(
|
|
listing=SkillListing(
|
|
id=f"owner/repo/{name}",
|
|
name=name,
|
|
description="A test skill",
|
|
author="Test Author",
|
|
source="github",
|
|
source_url=source_url,
|
|
tags=["test"],
|
|
),
|
|
parsed=ParsedSkill(
|
|
name=name,
|
|
description="A test skill",
|
|
content="# Test Skill\n\nInstructions here.",
|
|
tags=["test"],
|
|
author="Test Author",
|
|
version="1.0.0",
|
|
),
|
|
resources={"scripts/setup.sh": "#!/bin/bash\necho hello"},
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Tests: Discover
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestSkillDiscover:
|
|
def test_search_basic(self, client: TestClient) -> None:
|
|
listings = [_sample_listing()]
|
|
|
|
with patch("turnstone.core.skill_sources.SkillsShClient") as mock_cls:
|
|
instance = mock_cls.return_value
|
|
instance.search = AsyncMock(return_value=listings)
|
|
|
|
resp = client.get("/v1/api/admin/skills/discover?q=test")
|
|
|
|
assert resp.status_code == 200
|
|
data = resp.json()
|
|
assert len(data["skills"]) == 1
|
|
assert data["skills"][0]["name"] == "test-skill"
|
|
assert data["skills"][0]["installed"] is False
|
|
|
|
def test_search_empty_results(self, client: TestClient) -> None:
|
|
with patch("turnstone.core.skill_sources.SkillsShClient") as mock_cls:
|
|
instance = mock_cls.return_value
|
|
instance.search = AsyncMock(return_value=[])
|
|
|
|
resp = client.get("/v1/api/admin/skills/discover", params={"q": "test"})
|
|
|
|
assert resp.status_code == 200
|
|
assert resp.json()["skills"] == []
|
|
|
|
def test_search_empty_query_rejected(self, client: TestClient) -> None:
|
|
resp = client.get("/v1/api/admin/skills/discover")
|
|
assert resp.status_code == 400
|
|
|
|
def test_search_permission_denied(self, client_no_perm: TestClient) -> None:
|
|
resp = client_no_perm.get("/v1/api/admin/skills/discover")
|
|
assert resp.status_code == 403
|
|
|
|
def test_search_marks_installed(self, client: TestClient, storage: SQLiteBackend) -> None:
|
|
# Pre-install a skill with matching source_url
|
|
storage.create_prompt_template(
|
|
template_id="existing-id",
|
|
name="test-skill",
|
|
category="general",
|
|
content="existing content",
|
|
variables="[]",
|
|
is_default=False,
|
|
org_id="",
|
|
created_by="admin",
|
|
source_url="https://github.com/owner/repo",
|
|
)
|
|
|
|
listings = [_sample_listing()]
|
|
|
|
with patch("turnstone.core.skill_sources.SkillsShClient") as mock_cls:
|
|
instance = mock_cls.return_value
|
|
instance.search = AsyncMock(return_value=listings)
|
|
|
|
resp = client.get("/v1/api/admin/skills/discover?q=test")
|
|
|
|
assert resp.status_code == 200
|
|
assert resp.json()["skills"][0]["installed"] is True
|
|
|
|
def test_search_source_error(self, client: TestClient) -> None:
|
|
with patch("turnstone.core.skill_sources.SkillsShClient") as mock_cls:
|
|
instance = mock_cls.return_value
|
|
instance.search = AsyncMock(side_effect=SkillSourceError("timeout"))
|
|
|
|
resp = client.get("/v1/api/admin/skills/discover?q=test")
|
|
|
|
assert resp.status_code == 502
|
|
assert "timeout" in resp.json()["error"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Tests: Install
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestSkillInstall:
|
|
def test_install_from_github(self, client: TestClient) -> None:
|
|
package = _sample_package()
|
|
|
|
with patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch:
|
|
mock_fetch.return_value = package
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
|
|
assert resp.status_code == 200
|
|
data = resp.json()
|
|
assert data["total"] == 1
|
|
assert len(data["installed"]) == 1
|
|
skill = data["installed"][0]
|
|
assert skill["name"] == "test-skill"
|
|
assert skill["origin"] == "source"
|
|
assert skill["readonly"] is True
|
|
assert skill["source_url"] == "https://github.com/owner/repo"
|
|
|
|
def test_install_from_skills_sh(self, client: TestClient) -> None:
|
|
package = _sample_package()
|
|
|
|
with patch("turnstone.core.skill_sources.SkillsShClient") as mock_cls:
|
|
instance = mock_cls.return_value
|
|
instance.download_skill = AsyncMock(return_value=package)
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "skills.sh", "skill_id": "owner/repo/test-skill"},
|
|
)
|
|
|
|
assert resp.status_code == 200
|
|
assert resp.json()["installed"][0]["name"] == "test-skill"
|
|
|
|
def test_install_invalid_source(self, client: TestClient) -> None:
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "invalid"},
|
|
)
|
|
assert resp.status_code == 400
|
|
|
|
def test_install_missing_url(self, client: TestClient) -> None:
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github"},
|
|
)
|
|
assert resp.status_code == 400
|
|
|
|
def test_install_missing_skill_id(self, client: TestClient) -> None:
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "skills.sh"},
|
|
)
|
|
assert resp.status_code == 400
|
|
|
|
def test_install_duplicate_source_url(self, client: TestClient, storage: SQLiteBackend) -> None:
|
|
# Pre-install
|
|
storage.create_prompt_template(
|
|
template_id="existing-id",
|
|
name="existing-skill",
|
|
category="general",
|
|
content="content",
|
|
variables="[]",
|
|
is_default=False,
|
|
org_id="",
|
|
created_by="admin",
|
|
source_url="https://github.com/owner/repo",
|
|
)
|
|
|
|
package = _sample_package()
|
|
|
|
with patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch:
|
|
mock_fetch.return_value = package
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
|
|
assert resp.status_code == 409
|
|
|
|
def test_install_duplicate_name(self, client: TestClient, storage: SQLiteBackend) -> None:
|
|
# Pre-install with same name but different source_url
|
|
storage.create_prompt_template(
|
|
template_id="existing-id",
|
|
name="test-skill",
|
|
category="general",
|
|
content="content",
|
|
variables="[]",
|
|
is_default=False,
|
|
org_id="",
|
|
created_by="admin",
|
|
source_url="https://github.com/other/repo",
|
|
)
|
|
|
|
package = _sample_package(source_url="https://github.com/owner/different-repo")
|
|
|
|
with patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch:
|
|
mock_fetch.return_value = package
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/different-repo"},
|
|
)
|
|
|
|
assert resp.status_code == 409
|
|
|
|
def test_install_not_found(self, client: TestClient) -> None:
|
|
with (
|
|
patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch,
|
|
patch(
|
|
"turnstone.core.skill_sources.fetch_skills_from_github_repo",
|
|
new_callable=AsyncMock,
|
|
) as mock_batch,
|
|
):
|
|
mock_fetch.side_effect = SkillNotFoundError("SKILL.md not found")
|
|
mock_batch.side_effect = SkillNotFoundError("No SKILL.md files found")
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
|
|
assert resp.status_code == 404
|
|
|
|
def test_install_source_error_returns_502(self, client: TestClient) -> None:
|
|
with patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch:
|
|
mock_fetch.side_effect = SkillSourceError("connection timeout")
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
|
|
assert resp.status_code == 502
|
|
|
|
def test_install_permission_denied(self, client_no_perm: TestClient) -> None:
|
|
resp = client_no_perm.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
assert resp.status_code == 403
|
|
|
|
def test_install_stores_resources(self, client: TestClient, storage: SQLiteBackend) -> None:
|
|
package = _sample_package()
|
|
|
|
with patch(
|
|
"turnstone.core.skill_sources.fetch_skill_from_github", new_callable=AsyncMock
|
|
) as mock_fetch:
|
|
mock_fetch.return_value = package
|
|
|
|
resp = client.post(
|
|
"/v1/api/admin/skills/install",
|
|
json={"source": "github", "url": "https://github.com/owner/repo"},
|
|
)
|
|
|
|
assert resp.status_code == 200
|
|
skill_id = resp.json()["installed"][0]["template_id"]
|
|
resources = storage.list_skill_resources(skill_id)
|
|
assert len(resources) == 1
|
|
assert resources[0]["path"] == "scripts/setup.sh"
|