mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
06e16de066
Two related cleanups landed together because they touch the same surface
(skill-spec uplift PRs #569/#570/#571/#572):
1. Wording: replace "Anthropic spec" / "Anthropic Claude Code skill spec"
with "SKILL.md spec" across admin UI tooltips, code comments, test
docstrings, migration 056's module docstring, and the user-facing
`arguments` description in tools/skills.json. Renames a parser test
`test_anthropic_tags` -> `test_nested_metadata_tags` and consolidates
a parse-API test of the same shape; fixture author renamed
`Anthropic` -> `Acme` to keep the fixture neutral. Legitimate
provider/SDK/API references (provider name, api.anthropic.com,
`_anthropic.py`, capability comments) are intentionally untouched.
2. Admin UX: in the Create + Edit Skill modals, six fields per modal
(Compatibility, Paths, Hide-from-skill-picker, Arguments, Argument
hint, Activation) had long uppercase label-hint spans crammed into
the visible label. Migrated each to the existing
`.settings-help-btn` + `.settings-help-popover` pattern already used
in the Settings tab — short label + inline `?` button that opens a
styled popover with proper `<code>` formatting for technical tokens.
Pattern reuse required two small generalisations in admin.js:
* `_toggleSettingsHelp` now looks up the popover via a new
`data-help-target="<id>"` attribute first, falling back to the
settings-tab `.settings-label-col` ancestor lookup.
* `_closeAllSettingsHelp` mirrors the same dual-path lookup when
resetting `aria-expanded`, so modal buttons don't get stuck on
`aria-expanded="true"` after another popover opens.
* Added a document-delegated click handler that fires only for
buttons with `data-help-target`; existing per-button binding
in the settings-tab render path is unchanged.
CSS: `.settings-help-btn` now paints its `?` via `::after` with the
button's own `font-size: 0`, so prettier-introduced whitespace
inside the new HTML buttons can't off-center the glyph. The same
rule applies to existing admin.js-generated buttons (text content
hidden, pseudo identical). Small additions for
`.settings-help-popover code` / `strong` styling so technical
tokens render with the same monospace pill treatment used elsewhere
in skill UI.
Known follow-ups (intentionally NOT in this PR):
* Migrate the settings-tab `_renderSettingRow` button assembly to the
empty-`<button>` + `data-help-target` form so the per-button
addEventListener loop can be dropped in favour of pure document
delegation, and the `font-size: 0` rule stops being a workaround for
two markup styles.
* The 12 new popover blocks are duplicated verbatim between the
Create and Edit modals (same as the rest of the create/edit modal
pair). A small renderer that emits popovers from a shared data
object would eliminate the drift risk but is unrelated cleanup.
262 lines
9.0 KiB
Python
262 lines
9.0 KiB
Python
"""Tests for the SKILL.md parse admin API endpoint.
|
||
|
||
The endpoint is a thin permission-checked wrapper around
|
||
``turnstone.core.skill_parser.parse_skill_md``. These tests cover the
|
||
routing, auth, and error-handling layers — parser semantics live in
|
||
``test_skill_parser.py``.
|
||
"""
|
||
|
||
from __future__ import annotations
|
||
|
||
from typing import TYPE_CHECKING, Any
|
||
|
||
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 collections.abc import Iterator
|
||
|
||
from starlette.requests import Request
|
||
from starlette.responses import Response
|
||
|
||
from turnstone.console.server import admin_parse_skill
|
||
from turnstone.core.auth import AuthResult
|
||
|
||
|
||
class _InjectAuthMiddleware(BaseHTTPMiddleware):
|
||
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):
|
||
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)
|
||
|
||
|
||
_ROUTES = [
|
||
Mount(
|
||
"/v1",
|
||
routes=[
|
||
Route("/api/admin/skills/parse", admin_parse_skill, methods=["POST"]),
|
||
],
|
||
),
|
||
]
|
||
|
||
|
||
@pytest.fixture
|
||
def client() -> TestClient:
|
||
app = Starlette(
|
||
routes=_ROUTES,
|
||
middleware=[Middleware(_InjectAuthMiddleware)],
|
||
)
|
||
return TestClient(app)
|
||
|
||
|
||
@pytest.fixture
|
||
def client_no_perm() -> TestClient:
|
||
app = Starlette(
|
||
routes=_ROUTES,
|
||
middleware=[Middleware(_InjectAuthNoSkillsMiddleware)],
|
||
)
|
||
return TestClient(app)
|
||
|
||
|
||
_FULL_SKILL = """\
|
||
---
|
||
name: code-review
|
||
description: Automated code review skill
|
||
author: Test Author
|
||
version: 2.0.0
|
||
tags: [python, review, quality]
|
||
allowed-tools: [read_file, list_directory]
|
||
paths: ["**/*.py", "src/api/**"]
|
||
when_to_use: when the user asks to review code
|
||
model: claude-opus-4-7
|
||
effort: high
|
||
disable-model-invocation: true
|
||
user-invocable: false
|
||
arguments: [pr_number, focus]
|
||
argument-hint: "[pr-number] [focus-area]"
|
||
license: MIT
|
||
compatibility: ">=0.7"
|
||
---
|
||
|
||
# Code Review
|
||
|
||
Review code for best practices.
|
||
"""
|
||
|
||
_MINIMAL_SKILL = """\
|
||
---
|
||
name: minimal
|
||
---
|
||
|
||
Just some content.
|
||
"""
|
||
|
||
|
||
class TestParseSkill:
|
||
def test_parses_full_frontmatter(self, client: TestClient) -> None:
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": _FULL_SKILL})
|
||
assert resp.status_code == 200
|
||
data = resp.json()
|
||
assert data["name"] == "code-review"
|
||
# ``when_to_use`` is concatenated into description by the parser;
|
||
# the separate ``when_to_use`` field below shows the raw source.
|
||
assert data["description"] == (
|
||
"Automated code review skill\n\nWhen to use: when the user asks to review code"
|
||
)
|
||
assert data["author"] == "Test Author"
|
||
assert data["version"] == "2.0.0"
|
||
assert data["tags"] == ["python", "review", "quality"]
|
||
assert data["allowed_tools"] == ["read_file", "list_directory"]
|
||
assert data["paths"] == ["**/*.py", "src/api/**"]
|
||
assert data["when_to_use"] == "when the user asks to review code"
|
||
assert data["model"] == "claude-opus-4-7"
|
||
assert data["effort"] == "high"
|
||
assert data["disable_model_invocation"] is True
|
||
assert data["user_invocable"] is False
|
||
assert data["arguments"] == ["pr_number", "focus"]
|
||
assert data["argument_hint"] == "[pr-number] [focus-area]"
|
||
assert data["license"] == "MIT"
|
||
assert data["compatibility"] == ">=0.7"
|
||
assert "# Code Review" in data["content"]
|
||
# Frontmatter should not leak into the body.
|
||
assert "name: code-review" not in data["content"]
|
||
# ParsedSkill carries raw_frontmatter (the full YAML dict) but the
|
||
# handler whitelists fields by hand to avoid leaking arbitrary keys.
|
||
# Pin that contract — a future refactor to dataclasses.asdict would
|
||
# silently break it without this assertion.
|
||
assert "raw_frontmatter" not in data
|
||
|
||
def test_parses_minimal_frontmatter(self, client: TestClient) -> None:
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": _MINIMAL_SKILL})
|
||
assert resp.status_code == 200
|
||
data = resp.json()
|
||
assert data["name"] == "minimal"
|
||
assert data["description"] == "Just some content."
|
||
assert data["version"] == "1.0.0"
|
||
assert data["tags"] == []
|
||
assert data["allowed_tools"] == []
|
||
assert data["paths"] == []
|
||
assert data["when_to_use"] == ""
|
||
assert data["model"] == ""
|
||
assert data["effort"] == ""
|
||
# Spec defaults: model can autoload, user can pick.
|
||
assert data["disable_model_invocation"] is False
|
||
assert data["user_invocable"] is True
|
||
assert data["arguments"] == []
|
||
assert data["argument_hint"] == ""
|
||
assert data["license"] == ""
|
||
|
||
def test_nested_metadata_tags(self, client: TestClient) -> None:
|
||
# Some SKILL.md authors put tags under metadata.tags rather than
|
||
# at the top level — the parser must handle both layouts.
|
||
raw = """\
|
||
---
|
||
name: nested-meta
|
||
description: A skill using nested metadata
|
||
metadata:
|
||
tags: [alpha, beta]
|
||
author: Acme
|
||
version: 3.1.4
|
||
---
|
||
|
||
Body.
|
||
"""
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": raw})
|
||
assert resp.status_code == 200
|
||
data = resp.json()
|
||
assert data["tags"] == ["alpha", "beta"]
|
||
assert data["author"] == "Acme"
|
||
assert data["version"] == "3.1.4"
|
||
|
||
def test_unquoted_colon_in_description(self, client: TestClient) -> None:
|
||
# Common cross-client mistake: ``description: Use when: the user...``
|
||
# The parser retries with the description value quoted.
|
||
raw = """\
|
||
---
|
||
name: colon-desc
|
||
description: Use when: the user asks for a review
|
||
---
|
||
|
||
Body.
|
||
"""
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": raw})
|
||
assert resp.status_code == 200
|
||
data = resp.json()
|
||
assert data["name"] == "colon-desc"
|
||
assert "Use when" in data["description"]
|
||
|
||
def test_missing_name_returns_400(self, client: TestClient) -> None:
|
||
raw = """\
|
||
---
|
||
description: No name field
|
||
---
|
||
|
||
Body.
|
||
"""
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": raw})
|
||
assert resp.status_code == 400
|
||
assert "name" in resp.json()["error"].lower()
|
||
|
||
def test_missing_raw_returns_400(self, client: TestClient) -> None:
|
||
resp = client.post("/v1/api/admin/skills/parse", json={})
|
||
assert resp.status_code == 400
|
||
assert "raw" in resp.json()["error"].lower()
|
||
|
||
def test_blank_raw_returns_400(self, client: TestClient) -> None:
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": " \n"})
|
||
assert resp.status_code == 400
|
||
|
||
def test_invalid_yaml_returns_400(self, client: TestClient) -> None:
|
||
# YAML that the malformed-description retry can't fix.
|
||
raw = "---\nname: [not, valid, here\n---\nBody.\n"
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": raw})
|
||
assert resp.status_code == 400
|
||
|
||
def test_requires_admin_skills_permission(self, client_no_perm: TestClient) -> None:
|
||
resp = client_no_perm.post("/v1/api/admin/skills/parse", json={"raw": _MINIMAL_SKILL})
|
||
assert resp.status_code == 403
|
||
|
||
def test_oversized_content_length_returns_413(self, client: TestClient) -> None:
|
||
# Content-Length pre-check rejects oversized bodies before they're
|
||
# buffered into memory. Caps worker memory against an admin-token
|
||
# holder spraying multi-GB JSON. The threshold is generous (~4×
|
||
# the per-string cap) so payload here must clearly exceed it.
|
||
oversized = "a" * 200_000
|
||
resp = client.post("/v1/api/admin/skills/parse", json={"raw": oversized})
|
||
assert resp.status_code == 413
|
||
|
||
def test_oversized_raw_chunked_returns_413(self, client: TestClient) -> None:
|
||
# When the client sends Transfer-Encoding: chunked there is no
|
||
# Content-Length header, so the pre-check is skipped and the
|
||
# application-layer cap is the only line of defence. httpx switches
|
||
# to chunked when the body is a generator.
|
||
def _gen() -> Iterator[bytes]:
|
||
yield b'{"raw":"' + b"a" * 33_000 + b'"}'
|
||
|
||
resp = client.post(
|
||
"/v1/api/admin/skills/parse",
|
||
content=_gen(),
|
||
headers={"Content-Type": "application/json"},
|
||
)
|
||
assert resp.status_code == 413
|
||
assert "raw" in resp.json()["error"].lower()
|