6 Commits

Author	SHA1	Message	Date
Patrick Buckley	06e16de066	chore(skills): drop Anthropic attribution from SKILL.md spec references ... 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.	2026-05-24 14:56:43 -07:00
Patrick Buckley	9309162ac0	feat(skills): wire \$ARGUMENTS / \$N / \$<name> / \${CLAUDE_*} substitution (#572) ... Implements the Anthropic Claude Code skill spec's placeholder substitution end to end. The renderer in ``_substitute_skill_args`` handles every spec form except ``\${CLAUDE_SKILL_DIR}`` (deferred): * ``\$ARGUMENTS`` — full args string as the user/model typed it * ``\$ARGUMENTS[N]`` / ``\$N`` — Nth positional arg, ``shlex.split``-parsed * ``\$<name>`` — named arg from the SKILL.md ``arguments:`` list * ``\${CLAUDE_SESSION_ID}`` / ``\${CLAUDE_EFFORT}`` — session state Substitution is single-pass (one combined regex, one ``re.sub``). Append rule: when args are passed but the body has no bare ``\$ARGUMENTS``, append ``ARGUMENTS: …`` at the end. ## Surface * Parser: ``arguments:`` (list/space-delim) + ``argument-hint:`` (str) extracted into ``ParsedSkill``. * Install: persists both to the pre-allocated columns from migration 056 (PR #574). Install path clamps ``argument_hint`` to 128 chars to match the admin-create cap (untrusted upstream source). * Admin: ``CreateSkillRequest`` / ``UpdateSkillRequest`` accept both fields; create + edit modals get inputs; parse-preview echoes. * Renderer: ``_substitute_skill_args`` runs AFTER ``_render_template`` in ``_load_skills`` so user-supplied args containing ``{{var}}`` can't be re-expanded by the legacy renderer. * Session: ``_skill_arguments`` plumbed through ``__init__``, ``set_skill``, and ``_save_config`` so a resumed workstream re-renders with the original arg payload. * Model tool: ``skills(action='load')`` accepts an ``arguments`` string. Approval label includes a SHA-256 digest of the args so a once- approved skill name can't grant cover for a future payload; preview surfaces the args inline. ## ``/review`` findings (addressed) * ``\${CLAUDE_EFFORT}`` referenced ``self._reasoning_effort`` — wrong attribute; the real one is ``self.reasoning_effort``. Always rendered empty. Fixed. * Two-pass layering let user args containing ``{{var}}`` re-expand. Render order reversed. * ``_skill_arguments`` wasn't in ``_save_config`` — resumed workstreams silently lost their payload. Added. * Approval label omitted ``arguments``. Digest + preview added. * Install path didn't bound ``argument_hint``. Clamped. * Added ``_skill_arg_names`` decode tests + "load same skill, different args → re-render" invariant test. ## Copilot review findings (addressed) * ``skills.json`` tool description was inaccurate about ``shlex`` stripping quotes and "empty string disables substitution". Rewrote to match actual behaviour. * Named-argument regex was stricter than parser/storage contract. ``arguments: [issue-number]`` would partial-match ``\$issue-number`` as ``\$issue``, leaving ``-number`` as stray text. Broadened the regex to ``[A-Za-z_][A-Za-z0-9_]*`` AND added validation at ``_skill_arg_names`` decode time so names not matching the regex are dropped with a warning. ## Tests * ``tests/test_substitute_skill_args.py`` — placeholder forms, single-pass guarantee, append-at-end rule, shell-quoted input, unbalanced-quote fallback, uppercase + underscore-prefix names * ``tests/test_skill_parser.py::TestArgumentsAndHint`` — parser extraction * ``tests/test_skill_parse_api.py`` — HTTP parse-preview echoes both fields * ``tests/test_skill_discovery_api.py::test_install_seeds_arguments_and_argument_hint`` — install round-trip * ``tests/test_skills_tool.py::test_load_forwards_arguments_to_set_skill`` + ``test_load_same_skill_different_args_triggers_resub`` — wire path through prepare → exec → set_skill * ``tests/test_skills_tool.py::TestSkillArgNames`` — storage decode helper including the hyphen/dot/leading-digit filter	2026-05-23 17:48:16 -07:00
Patrick Buckley	6d28afbe7a	feat(skills): wire disable-model-invocation / user-invocable (#571) ... The Anthropic Claude Code skill spec defines two invocation-control axes Turnstone was parsing but not consuming: * ``disable-model-invocation: true`` — model can't autoload this skill (only user can invoke by name). Stored on ``ParsedSkill`` and echoed on the parse-preview UI; no install consumer because Turnstone hardcodes ``activation="named"`` on source-installs already. The dataclass docstring spells out the no-op so a future reader doesn't try to wire a translation that's already implicit. * ``user-invocable: false`` — skill stays available to the model but disappears from the user-facing picker. Mapped to ``hidden_from_menu=true`` on ``prompt_templates`` (column pre-allocated by PR #574); consumed by ``list_skills_summary`` (both the standalone-server and console-server impls). ## Surface * Parser: new ``_extract_bool`` helper accepts every YAML 1.1 boolean spelling (true/false/yes/no/on/off/1/0) plus their quoted variants — caught by ``/review`` as a real gap, since YAML's ``safe_load`` returns ``int`` for unquoted ``1``/``0`` and ``str`` for the YAML 1.1 spellings when quoted. * Install handler: derives ``hidden_from_menu`` from ``parsed.user_invocable`` on the source-install path. * Admin: ``CreateSkillRequest`` / ``UpdateSkillRequest`` accept ``hidden_from_menu``; both modals get a checkbox; the parse-preview auto-fill flips it when the source SKILL.md sets ``user-invocable: false``. * Runtime config: ``hidden_from_menu`` joined ``SKILL_RUNTIME_CONFIG_FIELDS`` so admin can override on installed (readonly) skills — same precedent as ``model`` / ``effort``. ## list_skills_summary shared helper Two identical implementations of ``list_skills_summary`` had accreted in ``turnstone/server.py`` and ``turnstone/console/server.py``. Both needed the new ``hidden_from_menu`` filter, so extracted the shared body to ``turnstone/core/web_helpers.skill_summary_rows``. Future spec-uplift fields (e.g. #572's ``argument_hint`` for autocomplete) only touch one place now. ## Tests * ``TestInvocationControl`` — bool / quoted / YAML 1.1 / int variants across both fields * ``test_install_user_invocable_false_sets_hidden_from_menu`` + default-unhidden case * ``test_list_skills_summary_excludes_hidden_from_menu`` — picker filter, admin tab unaffected * ``test_update_skill_readonly_hidden_from_menu_allowed`` — admin can hide/unhide installed skills via PUT (pins the runtime-config membership invariant) * Existing parse-API fixture extended with both new fields plus default-case assertions	2026-05-23 17:27:36 -07:00
Patrick Buckley	4c8f5acd3e	feat(skills): ingest when_to_use / model / effort from SKILL.md (#570) ... The Anthropic Claude Code skill spec defines three frontmatter fields the parser was previously dropping; this PR wires them through to the existing storage shape so the SKILL.md author's intent survives the import. * ``when_to_use`` — concatenated into ``description`` at parse time with a ``\n\nWhen to use: `` separator. Kept as its own field on ``ParsedSkill`` so the admin parse-preview UI can surface it separately. * ``model`` — passed through to ``create_prompt_template(model=...)`` on the source-install path, seeding the existing ``prompt_templates.model`` column. * ``effort`` — same shape, translates to the existing ``reasoning_effort`` column at the install handler boundary. Re-install short-circuits at the source_url dedup, so admin overrides to either column survive an upstream re-install — covered by a new ``test_reinstall_preserves_admin_model_override`` test that pins the load-bearing invariant. ## Description length cap ``_MAX_DESCRIPTION_LEN`` exported as ``MAX_SKILL_DESCRIPTION_LEN`` (public name) and raised from 1024 to 1536 to match the spec's combined ``description`` + ``when_to_use`` listing budget. All five write surfaces now import the same constant rather than each carrying their own magic number: * ``skill_parser.MAX_SKILL_DESCRIPTION_LEN`` — parse-time cap * ``console_schemas.CreateSkillRequest.description`` — Pydantic * ``console_schemas.UpdateSkillRequest.description`` — Pydantic * ``console/server.admin_create_skill`` — handler slice * ``console/server.admin_update_skill`` — handler slice * ``core/session._exec_skills_create`` — coordinator tool slice * ``core/session._exec_skills_update`` — coordinator tool slice The coordinator sites (last two) were the bug ``/review`` caught: they still capped at 1024 after the rest of the surface bumped to 1536, so a model-issued ``skills(action='create')`` with a 1025-1536 char description would silently truncate. Sharing the constant closes that desync. ## when_to_use truncation guard The ``when_to_use`` concat reserves room for the separator + at least one character of the appended value; below that budget, the addition is dropped entirely. Previously the naive concat could truncate mid-separator and leave the description ending in a dangling ``\n\nWhen ``. ## Tests * ``TestWhenToUse`` — concat semantics, no-description fallback, 1536 truncation * ``TestModelAndEffort`` — extraction + defaults * ``test_install_seeds_model_and_effort_from_frontmatter`` — install path persists both columns * ``test_install_no_model_or_effort_leaves_columns_empty`` — bare SKILL.md doesn't invent values * ``test_reinstall_preserves_admin_model_override`` — admin edits survive an upstream re-install (dedup invariant) * ``test_parses_full_frontmatter`` / ``test_parses_minimal_frontmatter`` extended with the new field assertions	2026-05-23 15:11:56 -07:00
Patrick Buckley	93af031fc7	feat(skills): parse and store Anthropic spec paths + uplift columns ... Implements PR1 of issue #569 — parser + storage + admin UI for the Anthropic Claude Code skill spec `paths:` SKILL.md frontmatter field (glob patterns gating model-initiated autoload). The autoload filter that consumes `paths` is deferred to a follow-up PR pending the workstream-CWD design discussion. Migration 056 bundles three additional columns whose consumer PRs are filed but not yet implemented: * `hidden_from_menu` (boolean) — backs the spec's `user-invocable: false` (issue #571). * `arguments` (JSON list) — backs spec `arguments:` named arg slots (issue #572). * `argument_hint` (string) — autocomplete display string (issue #572). The deferred columns surface in `SkillInfo` (response) so consumers can read them, but are deliberately absent from `CreateSkillRequest` and `UpdateSkillRequest` — the create/update handlers don't yet read them and advertising a writable field the handler would silently ignore would be an OpenAPI lie. Surface - Parser: `ParsedSkill.paths` populated from frontmatter; accepts the spec's YAML-list-or-CSV-string shape via the existing `_extract_list` machinery. - Storage: 4 new columns on `prompt_templates`; `SKILL_MUTABLE` extended; `_row_to_dict` calls extended to cast the new bool; protocol + SQLite + PostgreSQL `create_prompt_template` signatures threaded. - HTTP: admin create/update/install/parse handlers plumb `paths` through. Pydantic schemas extended accordingly. - Admin UI: `skill-paths` and `etm-paths` inputs on the create + edit modals; field map and read/write helpers wired across paste-parse, reset, create-send, edit-load, edit-send, and the readonly-disable list. Notable - `_canonicalize_skill_string_list` collapses the list-or-CSV-or-JSON- string normalization shared between admin_create_skill and admin_update_skill. Treats `None` as no-value so a body containing `{"paths": null}` doesn't CSV-split through `str(None)` and store the literal `["None"]`. Will back `arguments` once #572 wires its consumer. Tests - Parser: TestPaths covers YAML list, CSV string, empty, full- frontmatter integration (tests/test_skill_parser.py). - Storage: round-trip suite covers create + read + update for each of the four new columns on both backends (tests/test_storage_skill_spec_uplift.py). - Helper: focused unit tests for the canonicalizer including the regression-net case for the null-corruption bug (tests/test_canonicalize_skill_string_list.py). - HTTP boundary: extended test_parses_full_frontmatter + test_parses_minimal_frontmatter to assert `paths` survives the admin parse endpoint.	2026-05-23 14:20:36 -07:00
Patrick Buckley	0a8083e6d5	feat(skills): paste SKILL.md to auto-fill the Create Skill modal (#477) ... * feat(skills): paste SKILL.md to auto-fill the Create Skill modal When a user pastes an Anthropic-style SKILL.md (YAML frontmatter + markdown body) into the Create Skill content textarea, the frontend sniffs the leading ``---``, posts the raw text to a new backend parse endpoint, and populates name / description / tags / author / version / license / compatibility / allowed_tools from the parsed fields. The textarea is left with the body only (frontmatter stripped), and a toast reports how many fields were set vs. kept (already-typed values are preserved). Backend - ``POST /v1/api/admin/skills/parse`` (admin.skills permission) wraps the existing ``turnstone.core.skill_parser.parse_skill_md`` so admin imports and external installs share one parser. ``ParseSkillRequest`` / ``ParseSkillResponse`` schemas added; OpenAPI spec + sync/async console SDK methods updated. - Hardening: 32 KiB cap on ``raw`` (Pydantic ``max_length`` + handler enforcement); ``Content-Length`` pre-check returns 413 before any body buffering; parse offloaded via ``asyncio.to_thread`` so deeply-nested YAML cannot stall the event loop. Frontend (turnstone/console/static) - New paste handler with optimistic paint (raw text shown immediately, textarea disabled + ``aria-busy`` flipped, hint switches to "Parsing...") so the round-trip is visible on slow networks. - ``AbortController`` + generation guard (``_ctmPasteController``) so a fresh paste or modal close cancels a stale fetch — the previous handler's callbacks see the controller has been replaced and bail before touching the DOM. - Non-destructive overwrite: ``_setSkillFormField`` returns "filled" / "skipped" / "absent" and refuses to clobber non-empty values. Toast reports counts. - Bumps ``#toast`` z-index above modal overlays (was 200 vs. modal 600 — toasts fired while a modal was open were invisible). Console-wide fix exposed by this being the first feature to fire toasts mid-modal. HTML / CSS - New ``.skill-paste-hint`` line above the textarea announcing the affordance, sized to match surrounding ``.label-hint`` text. - ``aria-describedby`` ties the hint to the textarea; ``aria-live= "polite"`` announces the busy-state transition to screen readers. - "Skill Content" heading hint reworded "system message — ..." → "available: ..." and the variables row label "Variables" → "Used" to disambiguate available vs. in-use template variables. Tests - 11 new cases in ``tests/test_skill_parse_api.py``: happy paths (full / minimal / nested-metadata / unquoted-colon recovery), malformed YAML 400, missing/blank/missing-name 400, RBAC 403, raw body 32 KiB cap (Content-Length pre-check), chunked-encoding bypass forces the application-layer cap. Test pins ``raw_frontmatter`` omission so a future ``dataclasses.asdict`` refactor can't silently leak the full YAML dict back to clients. Validation - 5146 / 5146 ``pytest -k "not live"`` pass. - ``ruff`` + ``mypy`` clean on changed sources. - ``node -c`` clean on governance.js. - Two-stage code review (full pipeline + bug+quality re-review of the fix patches) applied; all confirmed findings addressed. * fix(skills): Copilot PR #477 review fixes (cumulative bug-1, bug-2, q-1) bug-1 (server.py): Content-Length pre-check was clamped to 32 KiB — the same number as the per-string char cap on ``raw``. A legitimate ``raw`` of exactly 32 KiB produces a JSON body well above 32 KiB once the ``{"raw":"..."}`` wrapper and any escaping is added, so valid near-max requests were 413'd. New constant ``_PARSE_SKILL_MAX_BODY_BYTES = _PARSE_SKILL_MAX_CHARS * 4`` admits the wrapper + multibyte expansion while still refusing obviously oversized payloads early; the per-string ``len(raw)`` check stays authoritative. bug-2 (governance.js): hideCreateTemplateModal aborted the inflight paste controller and nulled the global, but the handler's ``.catch`` and ``.finally`` guard each DOM mutation behind ``_isCurrent()`` — both bail when the controller has been nulled, leaving the textarea ``disabled`` + ``aria-busy`` and the hint stuck on "Parsing…". Reopening the modal landed on a poisoned state. The second-pass review's q-2 cleanup that dropped the show-side defensive reset missed this scenario — the verifier's reachability argument confused "controller is null" with "UI state is reset"; the two are independent. Hide now resets the paste-induced visible state alongside the abort. q-1 (console_spec.py): error_codes for the parse endpoint listed only 400; handler also returns 413 for oversized bodies. Added 413; kept 403 implicit per the convention sibling admin endpoints follow. Test fixup: bumped the Content-Length test payload to 200 KB so it clearly exceeds the new 128 KB pre-check threshold; otherwise it was falling through to the per-string check and duplicating test_oversized_raw_chunked_returns_413's coverage.	2026-05-04 16:12:53 -07:00