From 7c43c7bcbe4c7dab6324eac28a288da25b2d36ec Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 02:57:21 +0000 Subject: [PATCH 1/2] Add skilldeck new and skilldeck validate for skill authors `skilldeck new NAME --category ...` scaffolds a skill: a meta.yaml with every required field (version 0.1.0, every native agent unless --agent narrows it) and a skill.md skeleton carrying the shared review-skill structure (Scope diff steps, finding format, the severity rubric word for word, verify-before-reporting, findings cap, report header). Wherever domain content goes it writes a TODO(author) placeholder; it states no domain guidance and cites no source. In a checkout it also scaffolds evals/fixtures/NAME/ as a valid clean-diff fixture skeleton (--no-eval-fixture skips it). It never overwrites anything. `skilldeck validate [NAME|PATH]... [--skills-dir] [--json]` checks skills offline: meta.yaml (registry validation), structure, cited sources, leftover placeholders, stray files, rendering by every native and legacy adapter, and the catalog entry. For a checkout's src/skilldeck/skills it also checks eval fixtures (via the checkout's evals/run_evals.py loader), the skill's rows in docs/finding-output.md, and generated-output freshness (scripts/build_plugin.py --check logic, in process). Every problem names its file and line, a rule id, and a remediation; --json is deterministic; exit 0 only when clean, 2 on usage errors. Placeholder policy: a fresh skeleton passes every metadata and structure check, and is rated "incomplete" (not "invalid") while placeholders, a cited source, a planted eval fixture or its finding-output row are missing, rather than carrying a fake passing citation. Outside a checkout both commands need an explicit directory (--dir, --skills-dir) for organization skills, and new refuses to write into the installed package. The structure and citation rules move from the tests into skilldeck.lint (with a RULES table of ids, levels and remediations, and a package copy of the severity rubric that a test keeps identical to docs/finding-output.md); test_skill_structure and test_skill_citations now call the package rules. Registry SkillErrors carry the rule they break, with messages unchanged, and replacement checks are available without raising. catalog.skill_entry builds one entry without the packaged manifest. docs/authoring-skills.md now leads with the commands, lists every rule (a test keeps the table in sync), and documents the review path for official and organization-specific skills. README, CONTRIBUTING, CLAUDE.md and CHANGELOG updated. Closes #72 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HtiCGzpikMrkDYBkfQG5CX --- CHANGELOG.md | 26 + CLAUDE.md | 27 +- CONTRIBUTING.md | 15 +- README.md | 12 +- docs/authoring-skills.md | 262 +++++++++- docs/finding-output.md | 4 +- src/skilldeck/authoring.py | 905 ++++++++++++++++++++++++++++++++++ src/skilldeck/catalog.py | 13 + src/skilldeck/cli.py | 186 +++++++ src/skilldeck/lint.py | 560 +++++++++++++++++++++ src/skilldeck/registry.py | 237 ++++++--- tests/test_authoring.py | 648 ++++++++++++++++++++++++ tests/test_skill_citations.py | 65 ++- tests/test_skill_structure.py | 173 +++---- 14 files changed, 2896 insertions(+), 237 deletions(-) create mode 100644 src/skilldeck/authoring.py create mode 100644 src/skilldeck/lint.py create mode 100644 tests/test_authoring.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dd37c62..de0b0ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -516,6 +516,32 @@ All notable changes to this project are documented here. The format is based on ### Added +- Skill author commands (#72). `skilldeck new NAME --category ...` scaffolds + a skill: a `meta.yaml` with every required field (version `0.1.0`, every + agent unless `--agent` narrows it) and a `skill.md` skeleton with the shared + review-skill structure (Scope diff steps, finding format, the severity + rubric word for word, verify-before-reporting, findings cap, report + header). Wherever domain content goes it writes a `TODO(author)` + placeholder; it states no domain guidance and cites no source. In a + checkout it also scaffolds `evals/fixtures/NAME/` (skip with + `--no-eval-fixture`). `skilldeck validate [NAME|PATH]... [--skills-dir] + [--json]` checks skills offline: metadata, structure, cited sources, + leftover placeholders, stray files, rendering by every adapter (legacy + formats included) and the catalog entry; in a checkout also the eval + fixtures (loaded through `evals/run_evals.py`), the skill's row in + `docs/finding-output.md`, and generated-output freshness + (`scripts/build_plugin.py --check`, in process). Each problem names the + file and line, a rule id, and a fix; `--json` is deterministic, and the + exit status is 0 only when clean. A fresh skeleton passes every metadata + and structure check and is rated `incomplete` (not `invalid`) until its + placeholders, sources and eval fixture are written. Outside a checkout + both commands need an explicit directory (`--dir`, `--skills-dir`), for + organization skills, and `new` never writes into the installed package. + `docs/authoring-skills.md` now leads with the commands, lists every rule, + and documents the review path for official and organization skills. The + structure and citation rules moved from the tests into `skilldeck.lint`, + which the tests and `validate` share, and the registry's errors carry the + rule they break. - Agent compatibility matrix, `docs/compatibility.md` (#79), linked from the README and `docs/adapters.md`. It lists every adapter, including the `copilot-prompt`, `cursor-rule` and `kiro-steering` legacy adapters, with: diff --git a/CLAUDE.md b/CLAUDE.md index 076c5d7..d1b377d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,9 +49,23 @@ by `--agent all`); `skilldeck migrate` moves old-format installs to `SKILL.md`. `skill.md`); inside the package so they're bundled into the wheel - `src/skilldeck/` — the installer package - `cli.py` — `skilldeck list/show/install/uninstall/status/update/migrate`, - `provenance` and `catalog` + `provenance`, `catalog`, and the author commands `new` and `validate` - `registry.py` — discovers and validates skills, including the optional - `deprecated` metadata + `deprecated` metadata; each `SkillError` for a malformed skill carries + its `validate` rule id + - `lint.py` — the one home of the skill rules (structural template, severity + rubric copy, citation hygiene, placeholders) and the `RULES` table of every + `validate` rule id, level and remediation; `tests/test_skill_structure.py` + and `test_skill_citations.py` apply the same functions to the bundled skills. + Add a rule here, and to the rules table in `docs/authoring-skills.md` (a + test compares them) + - `authoring.py` — `skilldeck new` (scaffold, `TODO(author)` placeholders, no + domain guidance) and `skilldeck validate` (per-skill checks plus, in a + checkout's `src/skilldeck/skills`, eval fixtures, `docs/finding-output.md` + and generated-output freshness via the checkout's own `evals/run_evals.py` + and `scripts/build_plugin.py`, imported in process). Outside a checkout it + needs an explicit `--dir`/`--skills-dir` and never writes into the + installed package - `catalog.py` + `catalog.schema.json` — the public, schema-versioned `skilldeck catalog --json` contract (the schema ships in the wheel); change it only per the compatibility rules in `docs/catalog.md` (bump @@ -114,15 +128,16 @@ by `--agent all`); `skilldeck migrate` moves old-format installs to `SKILL.md`. are maintained by hand (see `docs/compatibility.md#contract-tests`). Only state vendor behaviour a primary source confirms; mark the rest *unverified*. -- New skills follow the structural template (enforced by - `tests/test_skill_structure.py`), ground their checklists in **fetched** +- New skills start from `skilldeck new`, follow the structural template + (the `skilldeck.lint` rules, which `skilldeck validate` reports and + `tests/test_skill_structure.py` enforces), ground their checklists in **fetched** authoritative sources (OWASP/CIS/vendor docs) cited in the skill body, and land with a golden-diff eval fixture under `evals/fixtures/` (ideally also a `-clean` one). - Review skills report in the shared shape of `docs/finding-output.md` and inline its one-paragraph severity rubric word for word in `## Output` - (`tests/test_skill_structure.py` compares them); change the rubric in the doc - and every skill together. Respect its "Which skill owns what" table: a + (`tests/test_skill_structure.py` compares them); change the rubric in the doc, + `skilldeck.lint.SEVERITY_RUBRIC` and every skill together. Respect its "Which skill owns what" table: a defect is reported once, by its owning skill. ## Shipping diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0db531a..e586e9a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,10 +48,17 @@ create them. ## Authoring or changing a skill Skills live in `src/skilldeck/skills//` as a `meta.yaml` + `skill.md`, authored -once in an agent-neutral format — never hand-edit per-agent output. A skill's -`meta.yaml` `name` must match its directory name. See -[docs/authoring-skills.md](docs/authoring-skills.md) for the full guide, and bump that -skill's own `version` in `meta.yaml` whenever its content changes. +once in an agent-neutral format — never hand-edit per-agent output. Scaffold a new one +and check your work with: + +```bash +uv run --extra dev skilldeck new my-review --category security +uv run --extra dev skilldeck validate my-review # file, rule and fix per problem +``` + +See [docs/authoring-skills.md](docs/authoring-skills.md) for the full guide and the +review path, and bump that skill's own `version` in `meta.yaml` whenever its content +changes. ## Opening the pull request diff --git a/README.md b/README.md index 24e84ef..d2e2d41 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,10 @@ skilldeck provenance --verify # Machine-readable, schema-versioned skill catalog for tools (filters optional) skilldeck catalog --json skilldeck catalog --json --category security --agent claude + +# Author a skill: scaffold it, then check it against every rule, offline +skilldeck new my-review --category security --dir skills +skilldeck validate --skills-dir skills my-review ``` `skilldeck catalog --json` is a stable contract for tools; see @@ -208,8 +212,12 @@ release; the package remains unpublished today. ## Authoring skills Each skill is a directory under `src/skilldeck/skills/` containing a `meta.yaml` -and a `skill.md`. See [docs/authoring-skills.md](docs/authoring-skills.md), and -follow the [contributor guide](CONTRIBUTING.md) for setup and validation. +and a `skill.md`. Start one with `skilldeck new` and check it with +`skilldeck validate`, which names the file, rule and fix for every problem; in +your own repository, pass `--dir` / `--skills-dir` to keep organization skills +there. See [docs/authoring-skills.md](docs/authoring-skills.md), including the +review path for official and organization skills, and follow the +[contributor guide](CONTRIBUTING.md) for setup. ## Changelog diff --git a/docs/authoring-skills.md b/docs/authoring-skills.md index 64fcd58..1d73213 100644 --- a/docs/authoring-skills.md +++ b/docs/authoring-skills.md @@ -1,16 +1,249 @@ # Authoring skills -A skill is a directory under `src/skilldeck/skills/` named after the skill. Living -inside the package means the skills are bundled into the wheel automatically, so a -`pip install skilldeck` ships them: +A skill is a directory named after the skill, holding two files: ``` -src/skilldeck/skills/ -└── my-skill/ - ├── meta.yaml - └── skill.md +my-review/ +├── meta.yaml # name, description, category, version, supported agents +└── skill.md # the agent-neutral instructions ``` +The bundled skills live in `src/skilldeck/skills/`, inside the package, so +they ship in the wheel. Two commands do the mechanical part of writing one: +`skilldeck new` scaffolds a skill that already has every required field and +section, and `skilldeck validate` checks a skill against every rule on this +page, offline, naming the file, the rule and the fix for each problem. Use +them as the way in; the sections after them describe the rules they apply. + +## Contributing a skill to this repository + +Open an issue first (see [CONTRIBUTING.md](../CONTRIBUTING.md)), then, in a +checkout: + +```bash +uv run --extra dev skilldeck new my-review --category security \ + --description "Review pending changes for ..." +# write the content: replace every TODO(author) placeholder in +# src/skilldeck/skills/my-review/skill.md, plant a defect in +# evals/fixtures/my-review/, list the skill in docs/finding-output.md +uv run --extra dev skilldeck validate my-review +uv run --extra dev python scripts/build_plugin.py # regenerate the plugin tree +uv run --extra dev skilldeck validate # every skill, before you push +``` + +Run the commands from the checkout with `uv run --extra dev`, so they use the +checkout's own code rather than an installed skilldeck (`validate` notes it +when they differ). + +## Organization skills + +Skills your organization keeps for itself live in your own repository, not +in this one. Outside a skilldeck checkout there is no default skills +directory, so name yours explicitly: + +```bash +skilldeck new my-review --category security --dir skills +skilldeck validate --skills-dir skills # every skill in it +skilldeck validate --skills-dir skills my-review # one skill, by name +skilldeck validate ./skills/my-review --json # or by path, for CI +``` + +skilldeck never writes into its own installed package: `new` refuses a +directory inside it. The checks that only make sense in this repository (eval +fixtures, `docs/finding-output.md`, the generated plugin tree) are skipped, +and the report says so. `skilldeck install` installs only the skills bundled +with skilldeck; it cannot install from your directory yet. + +## What `skilldeck new` writes + +```bash +skilldeck new NAME --category CATEGORY [--description TEXT] [--agent AGENT]... \ + [--dir PATH] [--no-eval-fixture] +``` + +- `NAME/meta.yaml` with every required field: the name, `--description` (a + placeholder if omitted), `--category`, version `0.1.0`, and + `supported-agents` listing every agent unless `--agent` names some. +- `NAME/skill.md`: the structure every review skill shares (see + [`skill.md`](#skillmd)): a title spelling the name, the `## Scope` steps + that determine the diff, and an `## Output` section with the finding + format, the severity rubric word for word, the verify-before-reporting + instruction, the findings cap and the report header. Wherever domain + content goes (what the skill reviews, the checklist, the sources it rests + on, the classifier and a worked example), it writes a `TODO(author)` + placeholder instead. The template states no domain guidance and cites no + source: that is the author's work, grounded in sources they fetched. +- In a checkout, `evals/fixtures/NAME/`: an `expected.yaml` and placeholder + `base/` and `change/` files, which you turn into a fixture with a planted + defect (see [`evals/README.md`](../evals/README.md#adding-a-fixture)). + `--no-eval-fixture` skips it. + +It never overwrites anything: an existing skill or fixture directory is an +error. All files are UTF-8 with LF line endings. + +### A new skill is incomplete, not invalid + +The skeleton passes every metadata and structure check as soon as it is +written. What it cannot pass without real content is reported honestly +rather than faked: `validate` rates the skill **incomplete** while +`TODO(author)` placeholders remain, no source is linked yet, or (in a +checkout) no eval fixture plants a defect or `docs/finding-output.md` does +not list it. An incomplete skill still fails validation (exit 1); it is just +told apart from one that breaks a rule (**invalid**). + +## What `skilldeck validate` checks + +```bash +skilldeck validate [NAME|PATH]... [--skills-dir PATH] [--json] +``` + +Give skill names from the skills directory (`--skills-dir`, by default the +checkout's `src/skilldeck/skills`), or paths to skill directories: anything +containing a slash, such as `./my-review`. With neither, every skill in the +skills directory is checked. For each skill it checks: + +- `meta.yaml`, with the same validation that loads skills for `install`; +- the `skill.md` structure and cited sources, the rules in `skilldeck.lint` + that the test suite also applies to every bundled skill; +- that no `TODO(author)` placeholder is left, and that the directory holds + nothing but the two skill files; +- that every adapter for the skill's agents, legacy formats included, can + render it, and that its [catalog](catalog.md) entry builds. + +A skill in a checkout's `src/skilldeck/skills` also gets the repository +checks: its eval fixtures load, have `base/` and `change/` with every planted +file in `change/`, and include a planted one (through `evals/run_evals.py`, +without running any agent); it is listed in +`docs/finding-output.md`; and the generated plugin tree and content manifests +are current (`scripts/build_plugin.py --check`, run in process). Every check +reads local files only; nothing touches the network or calls an agent. + +Each problem names the file (and line, where there is one), the rule and how +to fix it: + +``` +src/skilldeck/skills/my-review/skill.md:14: error [structure.scope] ## Scope lacks a three-dot diff against origin/ (/`git diff origin/\.\.\.HEAD`/) + fix: in ## Scope, determine the diff with `git fetch`, then `git diff origin/...HEAD`, plus uncommitted changes and untracked files (`git ls-files --others --exclude-standard`) +``` + +followed by each skill's status (`ok`, `incomplete` or `invalid`) and a +summary. The exit status is 0 when there is no problem at all, 1 when there +is any, and 2 for a usage error such as an unknown skill name. + +`--json` prints the same report for tools, as deterministic JSON (sorted +keys, problems sorted by skill, file, line and rule, one final newline): + +```json +{ + "notes": [], + "ok": false, + "problems": [ + { + "level": "incomplete", + "line": 3, + "message": "12 TODO(author) placeholders remain (lines 3, 4, 7, ...)", + "path": "src/skilldeck/skills/my-review/skill.md", + "remediation": "replace every TODO(author) placeholder with the real content", + "rule": "content.placeholder", + "skill": "my-review" + } + ], + "schema_version": 1, + "skills": [ + { + "checkout": true, + "name": "my-review", + "path": "src/skilldeck/skills/my-review", + "status": "incomplete" + } + ], + "skipped": [] +} +``` + +A problem that belongs to the whole checkout (a stale generated file) has +`skill: null`. `skipped` lists checks that could not apply, and why; +`notes` gives advice that is not a problem, such as a `validate` that runs +other code than the checkout's. `schema_version` changes only if the shape +changes incompatibly. + +### Rules + +| Rule | Level | Requires | +| --- | --- | --- | +| `meta.missing` | error | The skill directory has a `meta.yaml`. | +| `meta.encoding` | error | `meta.yaml` is UTF-8. | +| `meta.syntax` | error | `meta.yaml` is a YAML mapping. | +| `meta.missing-field` | error | `meta.yaml` has every required field. | +| `meta.unknown-field` | error | `meta.yaml` has only the known fields. | +| `meta.name` | error | `name` is 1–64 lowercase letters, digits and single hyphens. | +| `meta.name-mismatch` | error | `name` matches the skill's directory name. | +| `meta.description` | error | `description` is one non-empty line of at most 1024 characters. | +| `meta.description-period` | error | `description` is one sentence, ending with a period. | +| `meta.category` | error | `category` is a non-empty string. | +| `meta.version` | error | `version` is a `MAJOR.MINOR.PATCH` string. | +| `meta.supported-agents` | error | `supported-agents` is a non-empty list of distinct agent names. | +| `meta.unknown-agent` | error | `supported-agents` names only agents skilldeck has adapters for. | +| `meta.deprecated` | error | `deprecated`, when present, is a valid [deprecation record](#deprecating-a-skill). | +| `meta.deprecated-replacement` | error | A deprecated skill's replacement is a current skill in the same directory that supports the same agents. | +| `body.missing` | error | The skill directory has a `skill.md`. | +| `body.encoding` | error | `skill.md` is UTF-8. | +| `skill.unexpected-file` | error | The skill directory holds only `meta.yaml` and `skill.md`. | +| `structure.heading` | error | `skill.md` opens with a `# Title` whose words spell the skill name. | +| `structure.section` | error | `skill.md` has the `## Scope` and `## Output` sections. | +| `structure.phrase` | error | `skill.md` carries the instructions every review skill shares. | +| `structure.scope` | error | `## Scope` determines the diff the same way in every skill. | +| `structure.output` | error | `## Output` carries the shared finding-report pieces. | +| `structure.severity-rubric` | error | `## Output` inlines the shared severity rubric word for word. | +| `structure.nothing-in-scope` | error | `skill.md` says what to do when the change touches nothing in its area. | +| `structure.two-dot-range` | error | Git ranges are three-dot (`origin/...HEAD`). | +| `references.cited-source` | incomplete | `skill.md` links at least one authoritative source. | +| `references.superseded` | error | `skill.md` cites no superseded edition of a standard. | +| `references.redirect-url` | error | `skill.md` links no documentation path that only survives as a redirect. | +| `content.placeholder` | incomplete | No `TODO(author)` placeholder remains. | +| `render.failed` | error | Every adapter for the skill's agents can render it. | +| `catalog.entry` | error | The skill's catalog entry builds. | +| `eval.fixture-missing` | incomplete | Checkout only: an eval fixture with a planted defect exercises the skill. | +| `eval.fixture-invalid` | error | Checkout only: the skill's eval fixtures load, and their planted files are in `change/`. | +| `docs.finding-output` | incomplete | Checkout only: `docs/finding-output.md` lists the skill and its classifier. | +| `generated.stale` | error | Checkout only: the generated plugin tree and content manifests match the skills. | +| `generated.unchecked` | error | Checkout only: the generated-output check can run. | + +## Review path + +Passing `validate` is necessary, not sufficient: it proves a skill is well +formed, not that its advice is right. Nothing makes a skill official +automatically, and skilldeck generates no domain guidance of its own. + +**Official skills** join the bundled catalog only through a reviewed pull +request to this repository: + +1. Open an issue proposing the skill, and agree its scope and its overlap + with existing skills. +2. Scaffold it with `skilldeck new` and write the content. Ground every + checklist item in an authoritative source you fetched (OWASP, CIS, vendor + documentation) and cite it in the body; don't state what no source backs. +3. Add a golden-diff eval fixture with a planted defect (ideally also a + `-clean` one), its `SAMPLE_REPORTS` entry in + `tests/test_eval_fixtures.py`, and run it through the evals + ([`evals/README.md`](../evals/README.md)); record the pass rate in the pull + request. +4. Add the skill to [`docs/finding-output.md`](finding-output.md): the list + of review skills, the classifier table, and + [Which skill owns what](finding-output.md#which-skill-owns-what) if it + overlaps another skill. +5. Regenerate the plugin tree, add a `CHANGELOG.md` entry, and run + `skilldeck validate` and the full check suite in + [CONTRIBUTING.md](../CONTRIBUTING.md). +6. A maintainer reviews the sources, the severity anchors, the overlap with + other skills and the eval results, and merges it or asks for changes. + +**Organization-specific skills** stay in your own repository, under your own +review. Scaffold and check them with `--dir` and `--skills-dir` (see +[Organization skills](#organization-skills)); `skilldeck validate --json` +fits a CI gate. They are never added to the official catalog, however they +validate: to propose one upstream, follow the official path above. + ## `meta.yaml` ```yaml @@ -87,9 +320,14 @@ assumptions); the adapters add whatever wrapping each agent needs at install tim If the skill is a **review** skill that emits findings, make its `## Output` section follow the shared [finding output format](finding-output.md) so findings -from different skills stay consistent. `tests/test_skill_structure.py` checks -that every review skill carries the shared pieces: +from different skills stay consistent. The structure rules in +`skilldeck.lint` (reported by `skilldeck validate`, and applied to every +bundled skill by `tests/test_skill_structure.py`) check that every review +skill carries the shared pieces, all of which the `skilldeck new` skeleton +already has: +- a `# Title` first line whose words spell the skill name (`# My Review` for + `my-review`); - a `## Scope` section whose first step determines the diff the same way in every skill: `git fetch`, then `git diff origin/...HEAD`, plus uncommitted changes and untracked files @@ -109,8 +347,10 @@ skill one line that leaves the owner's area to the owner. ## Testing your skill ```bash -skilldeck list # should show your new skill +skilldeck validate my-skill # every rule, offline +skilldeck show my-skill --agent claude # the rendered file, for a bundled skill skilldeck install my-skill --agent claude --scope project ``` -Then inspect the rendered output under `.claude/skills/my-skill/SKILL.md`. +Then inspect the rendered output under `.claude/skills/my-skill/SKILL.md`, and +run the skill's eval fixtures against a real agent (`evals/README.md`). diff --git a/docs/finding-output.md b/docs/finding-output.md index e40a875..273dae4 100644 --- a/docs/finding-output.md +++ b/docs/finding-output.md @@ -81,7 +81,9 @@ rests on. (OWASP's matrix calls the low/low cell "Note"; skills fold it into `low` or drop it.) This is the one-paragraph form of the matrix, covering every cell, -that every skill inlines word for word: +that every skill inlines word for word (`skilldeck new` writes it and +`skilldeck validate` checks it from a copy in `skilldeck/lint.py`, which a +test keeps identical to this one): > Rate `severity` on the shared severity rubric, impact × likelihood: > **critical** — high impact (code execution, auth bypass, stolen credentials or diff --git a/src/skilldeck/authoring.py b/src/skilldeck/authoring.py new file mode 100644 index 0000000..be6e927 --- /dev/null +++ b/src/skilldeck/authoring.py @@ -0,0 +1,905 @@ +"""Skill authoring: ``skilldeck new`` scaffolds a skill, ``skilldeck validate`` +checks one. + +Both work on a *skills directory*: a folder of ``/meta.yaml`` + +``/skill.md`` skills. In a skilldeck checkout the default is the +checkout's ``src/skilldeck/skills``, and the repository-only checks apply +too: an eval fixture under ``evals/fixtures/``, the skill's row in +``docs/finding-output.md``, and freshness of the generated plugin tree and +content manifests. Anywhere else the directory must be given explicitly, so +an organization can author its own skills with the same checks; nothing here +writes into the installed package. + +Every check is local: files are read, adapters render in memory, and the +checkout's own ``evals/run_evals.py`` and ``scripts/build_plugin.py`` are +imported to validate fixtures and generated output. No network, no agent. +""" + +from __future__ import annotations + +import contextlib +import hashlib +import importlib.util +import re +import sys +from collections.abc import Iterable, Sequence +from dataclasses import dataclass, field +from pathlib import Path +from types import ModuleType + +import yaml + +from . import registry +from .adapters import ADAPTERS, ALL_ADAPTERS +from .catalog import skill_entry +from .lint import ( + ERROR, + INCOMPLETE, + PLACEHOLDER, + SEVERITY_RUBRIC, + Problem, + description_problems, + finding_output_problems, + placeholder_problems, + reference_problems, + structure_problems, +) +from .registry import ( + MAX_DESCRIPTION_LENGTH, + MAX_NAME_LENGTH, + NAME_RE, + Skill, + SkillError, + discover_skills, + load_skill, + replacement_errors, +) + +#: where a checkout keeps its canonical skills and its eval fixtures +SKILLS_SUBDIR = Path("src", "skilldeck", "skills") +FIXTURES_SUBDIR = Path("evals", "fixtures") +FINDING_OUTPUT_DOC = Path("docs", "finding-output.md") +_PROJECT_NAME_RE = re.compile(r'^name\s*=\s*"skilldeck"\s*$', re.M) +#: the only files a skill directory holds +SKILL_FILES = ("meta.yaml", "skill.md") +#: a new skill's first version +INITIAL_VERSION = "0.1.0" +#: bumped on a breaking change to ``skilldeck validate --json`` +REPORT_SCHEMA_VERSION = 1 + + +# -- where skills live ------------------------------------------------------------ + + +@dataclass(frozen=True) +class Checkout: + """A skilldeck source checkout, where the repository-only checks apply.""" + + root: Path + + @property + def skills_dir(self) -> Path: + return self.root / SKILLS_SUBDIR + + @property + def fixtures_dir(self) -> Path: + return self.root / FIXTURES_SUBDIR + + +def is_checkout(root: Path) -> bool: + """Whether ``root`` is the top of a skilldeck source checkout.""" + try: + text = (root / "pyproject.toml").read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return False + return ( + (root / SKILLS_SUBDIR).is_dir() + and (root / "scripts" / "build_plugin.py").is_file() + and bool(_PROJECT_NAME_RE.search(text)) + ) + + +def find_checkout(start: Path) -> Checkout | None: + """The skilldeck checkout ``start`` is in, if any.""" + start = start.resolve() + for candidate in (start, *start.parents): + if is_checkout(candidate): + return Checkout(candidate) + return None + + +def checkout_of(skills_dir: Path) -> Checkout | None: + """The checkout whose canonical skills directory is ``skills_dir``, if any. + + Only that directory gets the repository checks: another directory inside + a checkout (a scratch copy, an organization's skills) is checked on its + own. + """ + resolved = skills_dir.resolve() + if len(resolved.parents) < len(SKILLS_SUBDIR.parts): + return None + root = resolved.parents[len(SKILLS_SUBDIR.parts) - 1] + if root / SKILLS_SUBDIR == resolved and is_checkout(root): + return Checkout(root) + return None + + +def installed_package_dir() -> Path | None: + """The installed skilldeck package directory, unless it is a checkout's + ``src/skilldeck`` (an editable install), where authoring is expected.""" + bundled = Path(registry.DEFAULT_SKILLS_DIR) + if checkout_of(bundled) is not None: + return None + return bundled.resolve().parent + + +def display_path(path: Path, base: Path | None = None) -> str: + """``path`` as a POSIX path, relative to ``base`` (the working directory) + when it is under it.""" + base = (base or Path.cwd()).resolve() + absolute = (base / path).resolve() + try: + return absolute.relative_to(base).as_posix() + except ValueError: + return absolute.as_posix() + + +def skill_dirs(skills_dir: Path) -> list[Path]: + """The skill directories in ``skills_dir``, as discovery finds them.""" + return sorted( + child + for child in skills_dir.iterdir() + if child.is_dir() and not child.name.startswith(".") + ) + + +# -- skilldeck new -------------------------------------------------------------- + + +class _MetaDumper(yaml.SafeDumper): + """Indents list items under their key, as the bundled meta.yaml files do.""" + + def increase_indent(self, flow: bool = False, indentless: bool = False) -> None: + super().increase_indent(flow, False) + + +def title_for(name: str) -> str: + """A ``# Title`` for skill ``name`` whose words spell the name.""" + return " ".join(part.capitalize() for part in name.split("-")) + + +def meta_text(name: str, description: str, category: str, agents: Sequence[str]) -> str: + fields: dict[str, object] = { + "name": name, + "description": description, + "category": category, + "version": INITIAL_VERSION, + "supported-agents": list(agents), + } + return yaml.dump( + fields, + Dumper=_MetaDumper, + sort_keys=False, + default_flow_style=False, + allow_unicode=True, + width=float("inf"), + ) + + +_P = PLACEHOLDER + +#: the skill.md skeleton: the structure every review skill shares, and a +#: placeholder wherever domain content goes; it states no domain guidance +SKILL_TEMPLATE = f"""\ +# {{title}} + +Review the **pending changes on the current branch** for {_P}: the concern +this skill reviews, in a sentence or two. {_P}: name the skills it pairs +with and the areas it leaves to them. + +{_P}: name the authoritative sources the checklist below is grounded in, +as Markdown links to the pages you fetched (OWASP, CIS, vendor documentation). + +## Scope + +1. Determine the diff: `git fetch`, then `git diff origin/...HEAD` + (default base: `main`/`master`; with no remote, the local base), plus + uncommitted changes (`git diff HEAD`) and untracked files + (`git ls-files --others --exclude-standard`; read them whole). If you are + already on the base branch, review the uncommitted changes instead. +2. Review only changed files and the code paths they touch, but read the + whole function or file around each hunk, not just the diff. +3. {_P}: which files and changes are in this skill's area. + +## What to look for + +{_P}: the checklist, grouped by category under `###` headings, each item +grounded in one of the sources cited above. + +## Output + +Report each finding as a single list item: + +- **[severity] classifier** — `file:line` + **Issue:** what is wrong (and, for a security finding, how it could be + exploited). + **Fix:** the concrete change that resolves it. + +{{rubric}}{_P}: at most a short list of severity anchors for this skill's +domain, consistent with the rubric, or delete this line. The classifier is +{_P}: the taxonomy tag findings are labelled with. Order findings by +severity, highest first, and keep one issue per finding. For example: + +- **[{_P}: severity] {_P}: classifier** — `{_P}: file:line` + **Issue:** {_P}: a realistic issue this skill finds. + **Fix:** {_P}: its concrete fix. + +Verify before reporting: re-check each candidate against the surrounding +code, and drop any you cannot back with a concrete scenario. Prefer the few +findings that matter; if more than ~10 survive, report the ones worth a +human's time and summarize the rest in a line. + +Open the report with one line stating what was reviewed and the outcome, e.g. +`Reviewed origin/main...HEAD (3 files): 1 finding, worst medium.` If the +change touches nothing in this skill's area, say so and stop. If it +introduces no problems, say so explicitly rather than manufacturing findings. +""" + +PLACEHOLDER_DESCRIPTION = f"{_P}: say in one sentence what this skill reviews." + +FIXTURE_TEMPLATE = f"""\ +# Golden-diff eval fixture for {{name}}; see evals/README.md#adding-a-fixture. +# {_P}: put the pre-change tree in base/ and the change under review, with +# the defect(s) the skill must find planted in it, in change/. List each +# planted defect under plants, with keywords that describe the defect rather +# than echo the code; set max-findings; add the fixture's SAMPLE_REPORTS +# entry in tests/test_eval_fixtures.py; and delete both README.md files. +skill: {{name}} +plants: [] +max-findings: 0 +""" +FIXTURE_BASE_README = ( + f"{_P}: replace this file with the pre-change files the review needs as context.\n" +) +FIXTURE_CHANGE_README = ( + f"{_P}: replace this file with the changed files, including the planted " + "defect(s).\n" +) + + +def _one_line(what: str, value: str, limit: int | None = None) -> str: + if not value.strip() or value.splitlines() != [value]: + raise ValueError(f"{what} must be a single non-empty line") + if limit is not None and len(value) > limit: + raise ValueError(f"{what} is {len(value)} characters; the limit is {limit}") + return value + + +def scaffold( + name: str, + *, + category: str, + agents: Sequence[str], + skills_dir: Path, + description: str | None = None, + eval_fixture: bool = True, +) -> dict[Path, str]: + """The files ``skilldeck new`` writes for skill ``name``, in order. + + ``meta.yaml`` and a ``skill.md`` skeleton in ``skills_dir/name``; in a + checkout (when ``skills_dir`` is its canonical skills directory) and with + ``eval_fixture``, also an eval fixture skeleton in + ``evals/fixtures/name``. Raises ``ValueError`` for a bad argument, or when + anything it would write already exists or lies in the installed package. + """ + if len(name) > MAX_NAME_LENGTH or not NAME_RE.fullmatch(name): + raise ValueError( + f"invalid skill name {name!r}: use at most {MAX_NAME_LENGTH} " + "lowercase letters, digits and single hyphens, starting and ending " + "with a letter or digit" + ) + _one_line("--category", category) + if description is None: + description = PLACEHOLDER_DESCRIPTION + else: + _one_line("--description", description, MAX_DESCRIPTION_LENGTH) + if not description.endswith("."): + raise ValueError( + "--description should be one sentence ending with a period" + ) + unknown = [agent for agent in agents if agent not in ADAPTERS] + if not agents or unknown: + raise ValueError( + f"supported agents must be some of {', '.join(sorted(ADAPTERS))}" + ) + + package = installed_package_dir() + target = skills_dir.resolve() + if package is not None and (target == package or package in target.parents): + raise ValueError( + f"{display_path(skills_dir)} is inside the installed skilldeck " + "package, which skilldeck never writes into; pass --dir with your " + "own skills directory, or work in a skilldeck checkout" + ) + + skill_dir = skills_dir / name + files = { + skill_dir / "meta.yaml": meta_text(name, description, category, agents), + skill_dir / "skill.md": SKILL_TEMPLATE.format( + title=title_for(name), rubric=SEVERITY_RUBRIC + ), + } + checkout = checkout_of(skills_dir) + fixture_dir = None + if checkout is not None and eval_fixture: + fixture_dir = checkout.fixtures_dir / name + files[fixture_dir / "expected.yaml"] = FIXTURE_TEMPLATE.format(name=name) + files[fixture_dir / "base" / "README.md"] = FIXTURE_BASE_README + files[fixture_dir / "change" / "README.md"] = FIXTURE_CHANGE_README + + if skill_dir.exists() or skill_dir.is_symlink(): + raise ValueError( + f"{display_path(skill_dir)} already exists; skilldeck new never " + "overwrites a skill" + ) + if fixture_dir is not None and (fixture_dir.exists() or fixture_dir.is_symlink()): + raise ValueError( + f"{display_path(fixture_dir)} already exists; pass --no-eval-fixture " + "to leave it as it is" + ) + return files + + +def write_files(files: dict[Path, str]) -> None: + """Create each file (never replacing one), UTF-8 with LF line endings.""" + for path, text in files.items(): + path.parent.mkdir(parents=True, exist_ok=True) + with path.open("x", encoding="utf-8", newline="\n") as handle: + handle.write(text) + + +# -- skilldeck validate --------------------------------------------------------- + + +@dataclass(frozen=True) +class SkillStatus: + name: str + #: the skill directory, as :func:`display_path` shows it + path: str + #: whether the repository checks applied (a checkout's skills directory) + checkout: bool + #: ok, incomplete (authoring work remains, nothing wrong) or invalid + status: str + + +@dataclass(frozen=True) +class Skipped: + """Checks that could not apply, and why.""" + + check: str + reason: str + + +@dataclass +class Report: + skills: list[SkillStatus] = field(default_factory=list) + problems: list[Problem] = field(default_factory=list) + skipped: list[Skipped] = field(default_factory=list) + notes: list[str] = field(default_factory=list) + + @property + def ok(self) -> bool: + return not self.problems + + def to_json(self) -> dict[str, object]: + return { + "schema_version": REPORT_SCHEMA_VERSION, + "ok": self.ok, + "skills": [ + { + "name": skill.name, + "path": skill.path, + "checkout": skill.checkout, + "status": skill.status, + } + for skill in self.skills + ], + "problems": [ + { + "skill": problem.skill, + "path": problem.path, + "line": problem.line, + "rule": problem.rule, + "level": problem.level, + "message": problem.message, + "remediation": problem.remediation, + } + for problem in self.problems + ], + "skipped": [ + {"check": skipped.check, "reason": skipped.reason} + for skipped in self.skipped + ], + "notes": list(self.notes), + } + + +_SCRIPTS: dict[Path, ModuleType] = {} + + +def _load_script(path: Path) -> ModuleType: + """Import a checkout script (``evals/run_evals.py``, + ``scripts/build_plugin.py``) by path, once per process. + + The scripts put the checkout's directories on ``sys.path`` to import + their helpers; that is undone afterwards, since skilldeck itself is + already imported. + """ + path = path.resolve() + if path in _SCRIPTS: + return _SCRIPTS[path] + # the test suite's conftest has already loaded run_evals under its stem + existing = sys.modules.get(path.stem) + existing_file = getattr(existing, "__file__", None) + if existing is not None and existing_file and Path(existing_file).resolve() == path: + _SCRIPTS[path] = existing + return existing + digest = hashlib.sha256(str(path).encode("utf-8")).hexdigest()[:12] + name = f"_skilldeck_{path.stem}_{digest}" + spec = importlib.util.spec_from_file_location(name, path) + if spec is None or spec.loader is None: + raise ImportError(f"cannot load {path}") + module = importlib.util.module_from_spec(spec) + saved = sys.path[:] + # dataclasses resolve their module through sys.modules while it executes + sys.modules[name] = module + try: + spec.loader.exec_module(module) + except BaseException: + sys.modules.pop(name, None) + raise + finally: + sys.path[:] = saved + _SCRIPTS[path] = module + return module + + +def _status(problems: Iterable[Problem]) -> str: + levels = {problem.level for problem in problems} + if ERROR in levels: + return "invalid" + if INCOMPLETE in levels: + return "incomplete" + return "ok" + + +class _Checker: + """Validates the skills of one run, loading each directory's siblings and + each checkout's scripts only once.""" + + def __init__(self, base: Path) -> None: + self.base = base + self._siblings: dict[Path, list[Skill]] = {} + + def show(self, path: Path) -> str: + return display_path(path, self.base) + + def siblings(self, skills_dir: Path) -> list[Skill]: + """The skills in ``skills_dir`` that load, for replacement checks.""" + if skills_dir not in self._siblings: + loaded = [] + for child in skill_dirs(skills_dir): + try: + loaded.append(load_skill(child, set(ADAPTERS))) + except SkillError: + continue + self._siblings[skills_dir] = loaded + return self._siblings[skills_dir] + + def skill( + self, skill_dir: Path, checkout: Checkout | None + ) -> tuple[list[Problem], list[Skipped]]: + name = skill_dir.name + meta_path = skill_dir / "meta.yaml" + body_path = skill_dir / "skill.md" + problems: list[Problem] = [] + skipped: list[Skipped] = [] + + skill: Skill | None = None + try: + skill = load_skill(skill_dir, set(ADAPTERS)) + except SkillError as exc: + problems.append( + Problem( + self.show(exc.file or skill_dir), + exc.rule or "meta.syntax", + exc.detail, + skill=name, + ) + ) + reported = {problem.rule for problem in problems} + + body = skill.body if skill is not None else None + if body is None and body_path.is_file(): + try: + body = body_path.read_text(encoding="utf-8") + except UnicodeDecodeError as exc: + if "body.encoding" not in reported: + problems.append( + Problem( + self.show(body_path), + "body.encoding", + f"skill.md is not valid UTF-8: {exc}", + skill=name, + ) + ) + except OSError: + pass + elif body is None and "body.missing" not in reported: + problems.append( + Problem( + self.show(body_path), "body.missing", "missing skill.md", skill=name + ) + ) + if body is not None: + where = self.show(body_path) + problems += structure_problems(name, body, where) + problems += reference_problems(name, body, where) + problems += placeholder_problems(body, where, name) + # an unreadable meta.yaml is the registry's to report + with contextlib.suppress(OSError, UnicodeDecodeError): + problems += placeholder_problems( + meta_path.read_text(encoding="utf-8"), self.show(meta_path), name + ) + + if skill_dir.is_dir(): + problems += [ + Problem( + self.show(child), + "skill.unexpected-file", + f"{child.name} is not part of a skill (only " + f"{' and '.join(SKILL_FILES)} are)", + skill=name, + ) + for child in sorted(skill_dir.iterdir()) + if child.name not in SKILL_FILES + ] + + if skill is None: + skipped.append( + Skipped( + f"rendering and catalog entry of {name}", + "meta.yaml and skill.md must load first", + ) + ) + else: + problems += description_problems( + name, skill.description, self.show(meta_path) + ) + problems += self._rendering(skill, meta_path) + problems += [ + Problem( + self.show(meta_path), + exc.rule or "meta.deprecated-replacement", + exc.detail, + skill=name, + ) + for exc in replacement_errors( + [ + *(s for s in self.siblings(skill_dir.parent) if s.name != name), + skill, + ] + ) + if exc.file == skill_dir / "meta.yaml" + ] + + if checkout is not None: + doc = checkout.root / FINDING_OUTPUT_DOC + if doc.is_file(): + problems += finding_output_problems( + doc.read_text(encoding="utf-8"), name, self.show(doc) + ) + fixture_problems, fixture_skipped = self._fixtures(name, checkout) + problems += fixture_problems + skipped += fixture_skipped + return problems, skipped + + def _rendering(self, skill: Skill, meta_path: Path) -> list[Problem]: + """Render the skill for every adapter that takes it, then its catalog + entry (which renders it again for the native agents).""" + problems = [] + for adapter_name, adapter in sorted(ALL_ADAPTERS.items()): + if not adapter.supports(skill): + continue + try: + adapter.render(skill) + except (SkillError, ValueError, yaml.YAMLError) as exc: + problems.append( + Problem( + self.show(meta_path), + "render.failed", + f"the {adapter_name} adapter cannot render it: {exc}", + skill=skill.name, + ) + ) + if problems: + return problems + try: + skill_entry(skill) + except (SkillError, OSError, UnicodeDecodeError, ValueError) as exc: + problems.append( + Problem( + self.show(meta_path), + "catalog.entry", + f"cannot build its catalog entry: {exc}", + skill=skill.name, + ) + ) + return problems + + def _fixtures( + self, name: str, checkout: Checkout + ) -> tuple[list[Problem], list[Skipped]]: + """The eval fixtures that exercise skill ``name``: present, loadable, + buildable, free of placeholders, and at least one planted.""" + fixtures_dir = checkout.fixtures_dir + names = {child.name for child in skill_dirs(checkout.skills_dir)} | {name} + candidates = ( + [ + path + for path in sorted(fixtures_dir.iterdir()) + if path.is_dir() and _fixture_owner(path.name, names) == name + ] + if fixtures_dir.is_dir() + else [] + ) + problems: list[Problem] = [] + skipped: list[Skipped] = [] + runner_path = checkout.root / "evals" / "run_evals.py" + try: + runner: ModuleType | None = _load_script(runner_path) + except Exception as exc: + runner = None + skipped.append( + Skipped( + f"eval fixture structure of {name}", + f"cannot load {self.show(runner_path)}: {exc}", + ) + ) + planted = present = broken = 0 + for path in candidates: + expected = path / "expected.yaml" + if runner is None: + present += expected.is_file() + continue + try: + fixture = runner.load_fixture(path) + except runner.FixtureError as exc: + broken += 1 + problems.append( + Problem( + self.show(expected), + "eval.fixture-invalid", + str(exc).removeprefix(f"{expected}: "), + skill=name, + ) + ) + continue + if fixture.skill != name: + if path.name == name: + broken += 1 + problems.append( + Problem( + self.show(expected), + "eval.fixture-invalid", + f"skill is {fixture.skill!r}, but the directory is " + f"named for {name}; a fixture directory is named " + "for the skill it exercises", + skill=name, + ) + ) + continue + present += 1 + planted += bool(fixture.plants) + for message in runner.fixture_layout_problems(fixture): + broken += 1 + problems.append( + Problem( + self.show(path), + "eval.fixture-invalid", + str(message).removeprefix(f"{path.name}: "), + skill=name, + ) + ) + for file in sorted(path.rglob("*")): + if not file.is_file() or "__pycache__" in file.parts: + continue + try: + text = file.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + problems += placeholder_problems(text, self.show(file), name) + if broken: + return problems, skipped + where = self.show(fixtures_dir / name) + if not present: + problems.append( + Problem( + where, + "eval.fixture-missing", + f"no eval fixture exercises {name}", + skill=name, + hint=f"add {where}/ with expected.yaml, base/ and change/ " + "planting a defect the skill must find, and its " + "SAMPLE_REPORTS entry in tests/test_eval_fixtures.py; see " + "evals/README.md#adding-a-fixture", + ) + ) + elif runner is not None and not planted: + problems.append( + Problem( + where, + "eval.fixture-missing", + f"{name}'s eval fixtures plant no defect (only clean-diff " + "fixtures)", + skill=name, + hint=f"plant a defect in {where}/change/ (or in a " + f"{name}- fixture), list it under plants in " + "expected.yaml, and add its SAMPLE_REPORTS entry in " + "tests/test_eval_fixtures.py; see " + "evals/README.md#adding-a-fixture", + ) + ) + return problems, skipped + + def generated(self, checkout: Checkout) -> tuple[list[Problem], list[Skipped]]: + """``scripts/build_plugin.py --check``, in process.""" + try: + discover_skills(checkout.skills_dir, known_agents=set(ADAPTERS)) + except SkillError: + return [], [ + Skipped( + "generated-output check", + f"a skill in {self.show(checkout.skills_dir)} does not " + "load; fix it first", + ) + ] + script = checkout.root / "scripts" / "build_plugin.py" + try: + build = _load_script(script) + stale = build.stale( + build.generate(checkout.root, checkout.skills_dir), checkout.root + ) + except (Exception, SystemExit) as exc: + return [ + Problem( + self.show(script), + "generated.unchecked", + f"cannot generate the plugin tree to compare: {exc}", + ) + ], [] + messages = { + "missing": "generated file is missing", + "outdated": "generated file is out of date", + "unexpected": "file in the generated tree that no skill generates", + } + problems = [] + for entry in stale: + kind, _, relative = str(entry).partition(": ") + problems.append( + Problem( + self.show(checkout.root / relative), + "generated.stale", + messages.get(kind, str(entry)), + ) + ) + return problems, [] + + +def _fixture_owner(directory: str, names: Iterable[str]) -> str | None: + """The skill a fixture directory is named for: the longest skill name it + equals or extends with ``-``.""" + owners = [ + name for name in names if directory == name or directory.startswith(f"{name}-") + ] + return max(owners, key=len, default=None) + + +def validate(skill_paths: Sequence[Path], base: Path | None = None) -> Report: + """Check every skill directory in ``skill_paths``. + + A skill in a checkout's canonical skills directory also gets the + repository checks, and each such checkout's generated output is checked + once. Problems are sorted: by skill, then file, line and rule, with + checkout-wide ones last. ``base`` (default: the working directory) is + what paths are shown relative to. + """ + checker = _Checker((base or Path.cwd()).resolve()) + report = Report() + checkouts: dict[Path, Checkout] = {} + outside: set[Path] = set() + seen: set[Path] = set() + for path in skill_paths: + skill_dir = path.resolve() + if skill_dir in seen: + continue + seen.add(skill_dir) + checkout = checkout_of(skill_dir.parent) + if checkout is None: + outside.add(skill_dir.parent) + else: + checkouts[checkout.root] = checkout + problems, skipped = checker.skill(skill_dir, checkout) + report.problems += problems + report.skipped += skipped + report.skills.append( + SkillStatus( + name=skill_dir.name, + path=checker.show(skill_dir), + checkout=checkout is not None, + status=_status(problems), + ) + ) + for skills_dir in sorted(outside): + report.skipped.append( + Skipped( + "repository checks (eval fixtures, finding-output doc, generated " + f"output) for {checker.show(skills_dir)}", + "not the src/skilldeck/skills directory of a skilldeck checkout", + ) + ) + running = Path(__file__).resolve().parent + for root, checkout in sorted(checkouts.items()): + problems, skipped = checker.generated(checkout) + report.problems += problems + report.skipped += skipped + if running != (root / "src" / "skilldeck").resolve(): + report.notes.append( + f"this skilldeck runs from {checker.show(running)}, not from " + f"{checker.show(root / 'src' / 'skilldeck')}; run `uv run " + "--extra dev skilldeck validate` in the checkout so the checks " + "use its code" + ) + report.skills.sort(key=lambda skill: (skill.name, skill.path)) + report.problems.sort(key=Problem.sort_key) + report.skipped.sort(key=lambda skipped: (skipped.check, skipped.reason)) + return report + + +def format_report(report: Report) -> list[str]: + """The human-readable report, one output line per entry.""" + lines = [] + for problem in report.problems: + where = problem.path + (f":{problem.line}" if problem.line else "") + lines.append(f"{where}: {problem.level} [{problem.rule}] {problem.message}") + lines.append(f" fix: {problem.remediation}") + if report.problems: + lines.append("") + for skill in report.skills: + own = [p for p in report.problems if p.skill == skill.name] + errors = sum(p.level == ERROR for p in own) + incomplete = len(own) - errors + if skill.status == "ok": + detail = "" + elif skill.status == "incomplete": + detail = f" (no errors; {incomplete} authoring item(s) remain)" + else: + detail = f" ({errors} error(s), {incomplete} incomplete)" + lines.append(f"{skill.name}: {skill.status}{detail}") + for skipped in report.skipped: + lines.append(f"skipped {skipped.check}: {skipped.reason}") + for note in report.notes: + lines.append(f"note: {note}") + counts = { + status: sum(skill.status == status for skill in report.skills) + for status in ("ok", "incomplete", "invalid") + } + summary = ( + f"{len(report.skills)} skill(s): {counts['ok']} ok, " + f"{counts['incomplete']} incomplete, {counts['invalid']} invalid" + ) + shared = sum(problem.skill is None for problem in report.problems) + if shared: + summary += f"; {shared} generated-output problem(s)" + lines.append(summary) + return lines diff --git a/src/skilldeck/catalog.py b/src/skilldeck/catalog.py index 87c31b2..44c8ba1 100644 --- a/src/skilldeck/catalog.py +++ b/src/skilldeck/catalog.py @@ -99,6 +99,19 @@ def _entry(skill: Skill, digest: str) -> CatalogSkill: } +def skill_entry(skill: Skill) -> CatalogSkill: + """The catalog entry for ``skill``, its canonical digest recomputed from + its files and not checked against any content manifest. + + ``skilldeck validate`` uses it to check that a skill the package does not + ship yet can be described. Raises ``OSError`` or ``UnicodeDecodeError`` + if ``meta.yaml`` cannot be read, and ``SkillError`` if an adapter cannot + render the skill. + """ + meta_text = (skill.path / "meta.yaml").read_text(encoding="utf-8") + return _entry(skill, canonical_skill_digest(meta_text, skill.body)) + + def build_catalog( skills: Iterable[Skill], manifest: ContentManifest | None = None ) -> Catalog: diff --git a/src/skilldeck/cli.py b/src/skilldeck/cli.py index eeb43af..9cad73a 100644 --- a/src/skilldeck/cli.py +++ b/src/skilldeck/cli.py @@ -9,6 +9,7 @@ import click +from . import authoring from .adapters import ( ADAPTERS, ALL_ADAPTERS, @@ -18,6 +19,7 @@ LegacyAdapter, ) from .catalog import CatalogError, build_catalog, catalog_schema_text, filter_catalog +from .lint import PLACEHOLDER from .provenance import canonical_json, distribution_provenance, verify_bundled_skills from .registry import Skill, SkillError, discover_skills from .stamp import read as read_stamp @@ -797,6 +799,190 @@ def _migrate_blocker(old: Path, state: InstallState, force: bool) -> str | None: return None +_NO_SKILLS_DIR = ( + "not inside a skilldeck checkout, so there is no default skills directory" +) + + +@cli.command(name="new") +@click.argument("name") +@click.option( + "--category", required=True, help="The skill's category, e.g. security or review." +) +@click.option( + "--description", + default=None, + help="The one-sentence description (default: a placeholder to replace).", +) +@click.option( + "--agent", + "agents", + multiple=True, + type=click.Choice(sorted(ADAPTERS)), + help="An agent the skill supports; repeat for several (default: all).", +) +@click.option( + "--dir", + "skills_dir", + type=click.Path(file_okay=False, path_type=Path), + default=None, + help="The skills directory to create it in. Default: src/skilldeck/skills " + "of the skilldeck checkout you are in; required anywhere else.", +) +@click.option( + "--no-eval-fixture", + is_flag=True, + help="In a skilldeck checkout, don't scaffold evals/fixtures//.", +) +def new( + name: str, + category: str, + description: str | None, + agents: tuple[str, ...], + skills_dir: Path | None, + no_eval_fixture: bool, +) -> None: + """Scaffold a new skill: meta.yaml and a skill.md skeleton. + + The skeleton has the structure every review skill shares and a + TODO(author) placeholder wherever domain content goes; it states no + domain guidance. In a skilldeck checkout it also scaffolds an eval + fixture. skilldeck never writes into its installed package. + """ + if skills_dir is None: + checkout = authoring.find_checkout(Path.cwd()) + if checkout is None: + raise click.UsageError( + f"{_NO_SKILLS_DIR}: pass --dir PATH, e.g. your organization's " + "skills directory (skilldeck never writes into its installed " + "package)" + ) + skills_dir = checkout.skills_dir + try: + files = authoring.scaffold( + name, + category=category, + description=description, + agents=list(dict.fromkeys(agents)) or sorted(ADAPTERS), + skills_dir=skills_dir, + eval_fixture=not no_eval_fixture, + ) + authoring.write_files(files) + except ValueError as exc: + raise click.UsageError(str(exc)) from exc + except OSError as exc: + raise click.ClickException(f"cannot create {name}: {exc}") from exc + for path in files: + click.echo(f"created {authoring.display_path(path)}") + in_checkout = authoring.checkout_of(skills_dir) is not None + if in_checkout: + check = f"uv run --extra dev skilldeck validate {name}" + else: + check = ( + "skilldeck validate --skills-dir " + f"{authoring.display_path(skills_dir)} {name}" + ) + steps = [ + f"Replace every {PLACEHOLDER} placeholder. Ground the checklist in " + "sources you fetched and cite them as links; the template states no " + "domain guidance of its own.", + ] + if in_checkout: + steps += [ + f"Build the eval fixture in evals/fixtures/{name}/ (see " + "evals/README.md) and add the skill to docs/finding-output.md.", + "Regenerate the plugin tree: uv run --extra dev python " + "scripts/build_plugin.py", + ] + steps.append(f"Check it: {check}") + click.echo("\nNext:") + for number, step in enumerate(steps, start=1): + click.echo(f" {number}. {step}") + + +def _is_path_argument(target: str) -> bool: + """A skill directory path, as opposed to a skill name.""" + return target in (".", "..") or "/" in target or "\\" in target + + +@cli.command() +@click.argument("targets", nargs=-1, metavar="[NAME|PATH]...") +@click.option( + "--skills-dir", + type=click.Path(file_okay=False, path_type=Path), + default=None, + help="Where to find skills given by NAME, and every skill when none is " + "given. Default: src/skilldeck/skills of the skilldeck checkout you are in.", +) +@click.option( + "--json", + "as_json", + is_flag=True, + help="Emit the deterministic machine-readable report as JSON.", +) +def validate(targets: tuple[str, ...], skills_dir: Path | None, as_json: bool) -> None: + """Check skills against every authoring rule, offline. + + Checks meta.yaml, the skill.md structure and cited sources, leftover + placeholders, rendering by every adapter and the catalog entry; in a + skilldeck checkout also the eval fixture, docs/finding-output.md and the + generated plugin tree. Each problem names its file, rule and fix. Exits 0 + when clean, 1 on any problem. + + Give skill NAMEs from the skills directory, or PATHs to skill directories + (anything with a slash, e.g. ./my-skill); with neither, every skill in the + skills directory is checked. + """ + if skills_dir is None: + checkout = authoring.find_checkout(Path.cwd()) + default_dir = checkout.skills_dir if checkout is not None else None + elif not skills_dir.is_dir(): + raise click.UsageError(f"--skills-dir {skills_dir} is not a directory") + else: + default_dir = skills_dir + no_dir = ( + f"{_NO_SKILLS_DIR}: pass --skills-dir PATH, or skill directory paths " + "(e.g. ./my-skill)" + ) + paths: list[Path] = [] + for target in targets: + if _is_path_argument(target): + path = Path(target) + if not path.is_dir(): + raise click.UsageError(f"{target} is not a directory") + paths.append(path) + continue + if default_dir is None: + raise click.UsageError(no_dir) + path = default_dir / target + if not path.is_dir(): + hint = ( + f"; to check the directory {target}, pass ./{target}" + if Path(target).is_dir() + else "" + ) + raise click.UsageError( + f"no skill {target!r} in {authoring.display_path(default_dir)}{hint}" + ) + paths.append(path) + if not targets: + if default_dir is None: + raise click.UsageError(no_dir) + paths = authoring.skill_dirs(default_dir) + if not paths: + raise click.UsageError( + f"no skills in {authoring.display_path(default_dir)}" + ) + report = authoring.validate(paths) + if as_json: + _echo_json(report.to_json()) + else: + for line in authoring.format_report(report): + click.echo(line) + if not report.ok: + raise SystemExit(1) + + def main() -> None: try: cli() diff --git a/src/skilldeck/lint.py b/src/skilldeck/lint.py new file mode 100644 index 0000000..8f0c616 --- /dev/null +++ b/src/skilldeck/lint.py @@ -0,0 +1,560 @@ +"""The rules a skill's files must follow: one source of truth. + +``skilldeck validate`` reports these rules, and the test suite applies the +same functions to every bundled skill, so the structural template, the +citation hygiene and the placeholder check are defined once, here. + +The structure rules pin what every review skill carries (see +``docs/authoring-skills.md``): a ``## Scope`` section that determines the diff +the same way in every skill, and an ``## Output`` section with the shared +finding format, the severity rubric from ``docs/finding-output.md`` word for +word, a worked example, the verify-before-reporting instruction, the findings +cap and the one-line report header. They match stable phrases or loose +patterns rather than exact wording, so editing a skill stays low-friction; +the rubric is the one exact match. + +Every rule has an id, a level and a remediation in :data:`RULES`. An +``error`` is something wrong; ``incomplete`` means authoring work remains +(``TODO(author)`` placeholders, no cited source yet, no eval fixture), which +is what a freshly scaffolded skill reports until its content is written. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass + +#: marks content a skill author still has to write; ``skilldeck new`` puts it +#: wherever the template cannot know the domain, and validate reports it +PLACEHOLDER = "TODO(author)" + +ERROR = "error" +INCOMPLETE = "incomplete" + + +@dataclass(frozen=True) +class Rule: + level: str + #: what the rule requires, in one line + summary: str + #: how to fix a problem the rule reports + remediation: str + + +_AUTHORING = "docs/authoring-skills.md" + +#: every rule ``skilldeck validate`` can report, by id +RULES: dict[str, Rule] = { + # meta.yaml, as the registry validates it (skilldeck.registry) + "meta.missing": Rule( + ERROR, + "the skill directory has a meta.yaml", + "create meta.yaml with name, description, category, version and " + "supported-agents (skilldeck new writes one)", + ), + "meta.encoding": Rule(ERROR, "meta.yaml is UTF-8", "save meta.yaml as UTF-8"), + "meta.syntax": Rule( + ERROR, + "meta.yaml is a YAML mapping", + "fix the YAML so the file is a mapping of field: value lines", + ), + "meta.missing-field": Rule( + ERROR, + "meta.yaml has every required field", + "add the missing field(s); all of name, description, category, version " + f"and supported-agents are required ({_AUTHORING}#metayaml)", + ), + "meta.unknown-field": Rule( + ERROR, + "meta.yaml has only the known fields", + "remove the field or correct its spelling", + ), + "meta.name": Rule( + ERROR, + "name is 1-64 lowercase letters, digits and single hyphens", + "use a name like my-review: lowercase letters and digits, single " + "hyphens between them", + ), + "meta.name-mismatch": Rule( + ERROR, + "name matches the skill's directory name", + "rename the directory or change name so the two match", + ), + "meta.description": Rule( + ERROR, + "description is one non-empty line of at most 1024 characters", + "write the description as a single line (a folded block needs >-)", + ), + "meta.description-period": Rule( + ERROR, + "description is one sentence ending with a period", + "end the description with a period", + ), + "meta.category": Rule( + ERROR, + "category is a non-empty string", + "set category to a grouping such as security or review", + ), + "meta.version": Rule( + ERROR, + "version is a MAJOR.MINOR.PATCH string", + 'quote the version as MAJOR.MINOR.PATCH, e.g. version: "0.1.0"', + ), + "meta.supported-agents": Rule( + ERROR, + "supported-agents is a non-empty list of distinct agent names", + "list each agent once, e.g. supported-agents: [claude, codex]", + ), + "meta.unknown-agent": Rule( + ERROR, + "supported-agents names only agents skilldeck has adapters for", + "use only claude, codex, copilot, cursor and kiro; the legacy " + "adapters (copilot-prompt, cursor-rule, kiro-steering) follow their " + "agent's entry and are never listed", + ), + "meta.deprecated": Rule( + ERROR, + "deprecated, when present, has since, reason and optional replacement", + f"fix the deprecated record as {_AUTHORING}#deprecating-a-skill " + "describes, or remove it", + ), + "meta.deprecated-replacement": Rule( + ERROR, + "a deprecated skill's replacement is a current sibling skill that " + "supports the same agents", + "name a skill in the same directory that is not deprecated and " + "supports every agent this one does", + ), + # skill.md + "body.missing": Rule( + ERROR, + "the skill directory has a skill.md", + "create skill.md with the skill body (skilldeck new writes a skeleton)", + ), + "body.encoding": Rule(ERROR, "skill.md is UTF-8", "save skill.md as UTF-8"), + "skill.unexpected-file": Rule( + ERROR, + "the skill directory holds only meta.yaml and skill.md", + "move the file out of the skill directory: installs carry only " + "meta.yaml and skill.md, and provenance --verify rejects other files", + ), + "structure.heading": Rule( + ERROR, + "skill.md opens with a '# Title' whose words spell the skill name", + "start skill.md with a heading such as '# My Review' for my-review", + ), + "structure.section": Rule( + ERROR, + "skill.md has the ## Scope and ## Output sections", + f"add the section, following the structural template in {_AUTHORING}", + ), + "structure.phrase": Rule( + ERROR, + "skill.md carries the shared instructions every review skill has", + f"add the missing instruction; see the structural template in {_AUTHORING}", + ), + "structure.scope": Rule( + ERROR, + "## Scope determines the diff the same way in every skill", + "in ## Scope, determine the diff with `git fetch`, then " + "`git diff origin/...HEAD`, plus uncommitted changes and " + "untracked files (`git ls-files --others --exclude-standard`)", + ), + "structure.output": Rule( + ERROR, + "## Output carries the shared finding-report pieces", + "add the missing piece to ## Output, as in docs/finding-output.md", + ), + "structure.severity-rubric": Rule( + ERROR, + "## Output inlines the shared severity rubric word for word", + "copy the quoted rubric paragraph from docs/finding-output.md" + "#severity-rubric into ## Output unchanged", + ), + "structure.nothing-in-scope": Rule( + ERROR, + "skill.md says what to do when the change touches nothing in its area", + "add a line telling the agent, when the change touches nothing in the " + "skill's area, to say so and stop", + ), + "structure.two-dot-range": Rule( + ERROR, + "git ranges are three-dot (origin/...HEAD)", + "use a three-dot range, e.g. origin/...HEAD", + ), + "references.cited-source": Rule( + INCOMPLETE, + "skill.md links at least one authoritative source", + "ground the checklist in sources you fetched (OWASP, CIS, vendor " + "docs) and cite them as Markdown links in the body", + ), + "references.superseded": Rule( + ERROR, + "skill.md cites no superseded edition of a standard", + "cite the current edition named in the message", + ), + "references.redirect-url": Rule( + ERROR, + "skill.md links no documentation path that only survives as a redirect", + "link the canonical URL named in the message", + ), + "content.placeholder": Rule( + INCOMPLETE, + f"no {PLACEHOLDER} placeholder remains", + f"replace every {PLACEHOLDER} placeholder with the real content", + ), + # adapters and catalog + "render.failed": Rule( + ERROR, + "every adapter for the skill's agents can render it", + "change what the message names so the adapter can write it (the " + "legacy cursor-rule format cannot escape some descriptions)", + ), + "catalog.entry": Rule( + ERROR, + "the skill's catalog entry builds", + "fix the error in the message; skilldeck catalog --json must be able " + "to describe the skill", + ), + # checks that apply only in a skilldeck checkout + "eval.fixture-missing": Rule( + INCOMPLETE, + "an eval fixture with a planted defect exercises the skill", + "add an eval fixture that plants a defect the skill must find; see " + "evals/README.md#adding-a-fixture", + ), + "eval.fixture-invalid": Rule( + ERROR, + "the skill's eval fixtures load, and their planted files are in change/", + "fix the fixture as the message says; see evals/README.md#fixture-layout", + ), + "docs.finding-output": Rule( + INCOMPLETE, + "docs/finding-output.md lists the skill and its classifier", + "add the skill to docs/finding-output.md: the list of review skills " + "at the top and the per-skill classifier table (and to Which skill " + "owns what if it overlaps another skill)", + ), + "generated.stale": Rule( + ERROR, + "the generated plugin tree and content manifests match the skills", + "regenerate with `uv run --extra dev python scripts/build_plugin.py` " + "and commit the result; never edit generated files by hand", + ), + "generated.unchecked": Rule( + ERROR, + "the generated-output check can run", + "fix the error in the message, then validate again", + ), +} + + +@dataclass(frozen=True) +class Problem: + """One broken rule, located as precisely as possible.""" + + #: the file, as a POSIX path, relative to the working directory when under it + path: str + rule: str + message: str + #: 1-based line in ``path``, when the problem sits on one + line: int | None = None + #: the skill it belongs to; None for a checkout-wide problem + skill: str | None = None + #: a remediation specific to this problem, overriding the rule's + hint: str | None = None + + @property + def level(self) -> str: + return RULES[self.rule].level + + @property + def remediation(self) -> str: + return self.hint or RULES[self.rule].remediation + + def sort_key(self) -> tuple[bool, str, str, int, str, str]: + # skills by name first, checkout-wide problems last + return ( + self.skill is None, + self.skill or "", + self.path, + self.line or 0, + self.rule, + self.message, + ) + + +# -- the structural template -------------------------------------------------- + +#: the one-paragraph severity rubric every review skill's ``## Output`` +#: inlines word for word; docs/finding-output.md quotes the same paragraph +#: (a test keeps the two identical), and ``skilldeck new`` writes it +SEVERITY_RUBRIC = """\ +Rate `severity` on the shared severity rubric, impact × likelihood: +**critical** — high impact (code execution, auth bypass, stolen credentials or +bulk data, data loss, an outage), readily triggered (by anyone who can reach +it, or in routine operation); **high** — high impact behind a common +precondition (an authenticated user, a collaborator, a routine failure), or +medium impact (limited exposure, degraded service) readily triggered; +**medium** — high impact only under an unusual precondition, medium impact +behind a common one, or low impact readily triggered (a weakened defense +anyone can reach); **low** — medium impact only under an unusual +precondition, or low impact behind any precondition (most defense in depth +and hygiene). +""" + +# level-2 heading -> why it must be present +REQUIRED_HEADINGS = { + "Scope": "a Scope section saying what to review", + "Output": "an Output section with the finding format", +} +# phrase (matched against whitespace-normalized text) -> why it must be present +REQUIRED_PHRASES = { + "uncommitted": "diff determination covering uncommitted/untracked changes", + "For example:": "a worked example finding", + "Verify before reporting": "the verify-before-reporting instruction", + "Open the report with one line": "the one-line report header instruction", +} +# regex (matched against the whitespace-normalized section) -> why it must match +SCOPE_PATTERNS = { + r"`git fetch`": "a fetch so the diff uses an up-to-date remote base", + r"`git diff origin/\.\.\.HEAD`": "a three-dot diff against origin/", + r"`git (?:ls-files --others --exclude-standard|status --porcelain)`": ( + "a command that lists untracked files" + ), +} +OUTPUT_PATTERNS = { + r"shared severity rubric": "a reference to the shared severity rubric", + r"more than ~\d+ survive.*summarize the rest": "the findings-cap sentence", + r"`Reviewed origin/\w+\.\.\.HEAD \(": "a three-dot range in the header example", +} +NOTHING_IN_SCOPE = r"say so and stop" +# `main..feature`, `..HEAD`; not `...`, and not a `../` path +TWO_DOT_RANGE = re.compile(r"[\w/<>-]*[\w>]\.\.(?!\.)[\w<][\w/<>-]*") + + +def normalize(text: str) -> str: + """``text`` with every run of whitespace collapsed to one space.""" + return " ".join(text.split()) + + +def _lf(text: str) -> str: + return text.replace("\r\n", "\n").replace("\r", "\n") + + +def _line_at(text: str, index: int) -> int: + return text.count("\n", 0, index) + 1 + + +def _heading(body: str, heading: str) -> re.Match[str] | None: + return re.search(rf"^## {re.escape(heading)}[ \t]*$", body, re.M) + + +def section(body: str, heading: str) -> str: + """The text under ``## heading`` up to the next level-2 heading.""" + match = re.search( + rf"^## {re.escape(heading)}[ \t]*\n(.*?)(?=^## |\Z)", _lf(body), re.M | re.S + ) + return match.group(1) if match else "" + + +def heading_slug(title: str) -> str: + """The skill name a ``# Title`` heading spells: ``My Review`` -> my-review.""" + return re.sub(r"[^a-z0-9]+", "-", title.strip().lower()).strip("-") + + +def structure_problems(name: str, body: str, path: str = "skill.md") -> list[Problem]: + """Every structural-template rule the ``skill.md`` of skill ``name`` breaks.""" + body = _lf(body) + flat = normalize(body) + problems: list[Problem] = [] + + def add(rule: str, message: str, line: int | None = None) -> None: + problems.append(Problem(path, rule, message, line, name)) + + first = body.split("\n", 1)[0] + if not first.startswith("# "): + add("structure.heading", "skill.md must open with a '# Title' heading", 1) + elif heading_slug(first[2:]) != name: + add( + "structure.heading", + f"heading {first!r} does not spell the skill name {name!r}", + 1, + ) + headings = {heading: _heading(body, heading) for heading in REQUIRED_HEADINGS} + for heading, why in REQUIRED_HEADINGS.items(): + if headings[heading] is None: + add("structure.section", f"missing '## {heading}' ({why})") + for phrase, why in REQUIRED_PHRASES.items(): + if phrase not in flat: + add("structure.phrase", f"missing {phrase!r} ({why})") + for heading, rule, patterns in ( + ("Scope", "structure.scope", SCOPE_PATTERNS), + ("Output", "structure.output", OUTPUT_PATTERNS), + ): + found = headings[heading] + if found is None: + continue # reported once, as the missing section + text = normalize(section(body, heading)) + line = _line_at(body, found.start()) + for pattern, why in patterns.items(): + if not re.search(pattern, text): + add(rule, f"## {heading} lacks {why} (/{pattern}/)", line) + output = headings["Output"] + if output is not None and normalize(SEVERITY_RUBRIC) not in normalize( + section(body, "Output") + ): + add( + "structure.severity-rubric", + "## Output does not inline the severity rubric paragraph from " + "docs/finding-output.md word for word", + _line_at(body, output.start()), + ) + if not re.search(NOTHING_IN_SCOPE, flat, re.I): + add( + "structure.nothing-in-scope", + "no line says what to do when the change touches nothing in scope " + f"(/{NOTHING_IN_SCOPE}/)", + ) + for match in TWO_DOT_RANGE.finditer(body): + add( + "structure.two-dot-range", + f"two-dot range {match.group(0)!r}", + _line_at(body, match.start()), + ) + return problems + + +def description_problems( + name: str, description: str, path: str = "meta.yaml" +) -> list[Problem]: + """The description rules beyond what the registry checks.""" + if description.endswith("."): + return [] + return [ + Problem( + path, + "meta.description-period", + "description should be one sentence ending with a period", + skill=name, + ) + ] + + +# -- references ----------------------------------------------------------------- + +LINK_RE = re.compile(r"\]\((https://[^)\s]+)\)") + +# pattern -> what to cite instead +SUPERSEDED = { + r"\bA\d{2}:2021\b|/Top10/A\d{2}_2021-": "OWASP Top 10:2025 (owasp.org/Top10/2025/)", + r"\bASVS\s*v?4\.": "ASVS 5.0", + # the 2026 edition renumbered the entries, so a 2025 ID names a different risk + r"\bLLM\d{2}:2025\b": "OWASP Top 10 for LLM Applications 2026 (LLMxx:2026)", +} + +# old doc paths that only work through redirects -> the canonical form +STALE_URL_PREFIXES = { + "https://docs.gitlab.com/ee/": "https://docs.gitlab.com//", + "https://docs.github.com/en/actions/security-for-github-actions/": ( + "https://docs.github.com/en/actions/reference/security/..." + ), + "https://docs.github.com/en/actions/security-guides/": ( + "https://docs.github.com/en/actions/reference/security/..." + ), +} + + +def reference_problems(name: str, body: str, path: str = "skill.md") -> list[Problem]: + """Citation hygiene: a source is linked, and none is superseded or stale.""" + body = _lf(body) + problems: list[Problem] = [] + if not LINK_RE.search(body): + problems.append( + Problem( + path, + "references.cited-source", + "no authoritative source is linked (a Markdown link to an " + "https:// page)", + skill=name, + ) + ) + for pattern, current in SUPERSEDED.items(): + for match in re.finditer(pattern, body): + problems.append( + Problem( + path, + "references.superseded", + f"{match.group(0)!r} is superseded; cite {current}", + _line_at(body, match.start()), + name, + ) + ) + for match in LINK_RE.finditer(body): + url = match.group(1) + for prefix, canonical in STALE_URL_PREFIXES.items(): + if url.startswith(prefix): + problems.append( + Problem( + path, + "references.redirect-url", + f"{url} only works through a redirect; use {canonical}", + _line_at(body, match.start()), + name, + ) + ) + return problems + + +# -- placeholders and docs -------------------------------------------------------- + + +def placeholder_problems( + text: str, path: str, skill: str | None = None +) -> list[Problem]: + """One problem if ``text`` still holds :data:`PLACEHOLDER` markers.""" + lines = [ + number + for number, line in enumerate(_lf(text).split("\n"), start=1) + if PLACEHOLDER in line + ] + if not lines: + return [] + count = text.count(PLACEHOLDER) + shown = ", ".join(map(str, lines[:10])) + (", ..." if len(lines) > 10 else "") + noun = "placeholder remains" if count == 1 else "placeholders remain" + where = "line" if len(lines) == 1 else "lines" + return [ + Problem( + path, + "content.placeholder", + f"{count} {PLACEHOLDER} {noun} ({where} {shown})", + lines[0], + skill, + ) + ] + + +def finding_output_problems( + doc: str, name: str, path: str = "docs/finding-output.md" +) -> list[Problem]: + """Whether docs/finding-output.md lists review skill ``name``: in its + opening list of review skills and in its per-skill classifier table.""" + doc = _lf(doc) + intro = doc.split("\n## ", 1)[0] + table = section(doc, "Fields") + missing = [] + if f"`{name}`" not in intro: + missing.append("the list of review skills at the top") + if f"| `{name}` |" not in table: + missing.append("the per-skill classifier table") + if not missing: + return [] + return [ + Problem( + path, + "docs.finding-output", + f"{name} is not in {' or '.join(missing)}", + skill=name, + ) + ] diff --git a/src/skilldeck/registry.py b/src/skilldeck/registry.py index fce7659..be6cc08 100644 --- a/src/skilldeck/registry.py +++ b/src/skilldeck/registry.py @@ -48,7 +48,35 @@ class SkillError(Exception): - """Raised when a skill directory is malformed.""" + """Raised when a skill directory is malformed. + + For a malformed skill, ``rule`` names the metadata rule it breaks (e.g. + ``meta.version``; ``skilldeck validate`` lists them), ``file`` is the file + that breaks it, and ``detail`` is the message without the leading skill + directory. Other errors leave ``rule`` and ``file`` None. + """ + + def __init__( + self, + message: str, + *, + rule: str | None = None, + file: Path | None = None, + detail: str | None = None, + ) -> None: + super().__init__(message) + self.rule = rule + self.file = file + self.detail = message if detail is None else detail + + +def _invalid( + skill_dir: Path, rule: str, detail: str, file: str = "meta.yaml" +) -> SkillError: + """A :class:`SkillError` for ``skill_dir`` breaking metadata ``rule``.""" + return SkillError( + f"{skill_dir}: {detail}", rule=rule, file=skill_dir / file, detail=detail + ) @dataclass(frozen=True) @@ -83,54 +111,71 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> body_path = skill_dir / "skill.md" if not meta_path.is_file(): - raise SkillError(f"{skill_dir}: missing meta.yaml") + raise _invalid(skill_dir, "meta.missing", "missing meta.yaml") if not body_path.is_file(): - raise SkillError(f"{skill_dir}: missing skill.md") + raise _invalid(skill_dir, "body.missing", "missing skill.md", "skill.md") try: meta = yaml.safe_load(meta_path.read_text(encoding="utf-8")) or {} except UnicodeDecodeError as exc: - raise SkillError(f"{skill_dir}: meta.yaml is not valid UTF-8: {exc}") from exc + raise _invalid( + skill_dir, "meta.encoding", f"meta.yaml is not valid UTF-8: {exc}" + ) from exc except yaml.YAMLError as exc: - raise SkillError(f"{skill_dir}: meta.yaml is not valid YAML: {exc}") from exc + raise _invalid( + skill_dir, "meta.syntax", f"meta.yaml is not valid YAML: {exc}" + ) from exc if not isinstance(meta, dict): - raise SkillError(f"{skill_dir}: meta.yaml must be a YAML mapping") + raise _invalid(skill_dir, "meta.syntax", "meta.yaml must be a YAML mapping") missing = [f for f in REQUIRED_FIELDS if f not in meta] if missing: - raise SkillError(f"{skill_dir}: meta.yaml missing fields: {', '.join(missing)}") + raise _invalid( + skill_dir, + "meta.missing-field", + f"meta.yaml missing fields: {', '.join(missing)}", + ) unknown = sorted(str(key) for key in meta if key not in ALLOWED_FIELDS) if unknown: - raise SkillError( - f"{skill_dir}: meta.yaml has unknown field(s): {', '.join(unknown)}; " - f"the fields are {', '.join(ALLOWED_FIELDS)}" + raise _invalid( + skill_dir, + "meta.unknown-field", + f"meta.yaml has unknown field(s): {', '.join(unknown)}; " + f"the fields are {', '.join(ALLOWED_FIELDS)}", ) name = _require_str(skill_dir, meta, "name") if len(name) > MAX_NAME_LENGTH or not NAME_RE.fullmatch(name): - raise SkillError( - f"{skill_dir}: meta.yaml name {name!r} must be at most " + raise _invalid( + skill_dir, + "meta.name", + f"meta.yaml name {name!r} must be at most " f"{MAX_NAME_LENGTH} lowercase letters, digits and single hyphens, " - "starting and ending with a letter or digit" + "starting and ending with a letter or digit", ) if name != skill_dir.name: - raise SkillError( - f"{skill_dir}: meta.yaml name '{name}' " - f"does not match directory name '{skill_dir.name}'" + raise _invalid( + skill_dir, + "meta.name-mismatch", + f"meta.yaml name '{name}' does not match directory name '{skill_dir.name}'", ) description = _require_str(skill_dir, meta, "description") # Any line boundary ``str.splitlines`` knows, not just \n and \r: YAML - # double-quoted escapes such as "\u2028" or "\x85" also break the one-line + # double-quoted escapes such as "
" or "\x85" also break the one-line # ``skilldeck list`` output. if description.splitlines() != [description]: - raise SkillError( - f"{skill_dir}: meta.yaml description must be a single line (a folded " - "block needs >- rather than >, which keeps a final line break)" + raise _invalid( + skill_dir, + "meta.description", + "meta.yaml description must be a single line (a folded " + "block needs >- rather than >, which keeps a final line break)", ) if len(description) > MAX_DESCRIPTION_LENGTH: - raise SkillError( - f"{skill_dir}: meta.yaml description is {len(description)} characters; " - f"the limit is {MAX_DESCRIPTION_LENGTH}" + raise _invalid( + skill_dir, + "meta.description", + f"meta.yaml description is {len(description)} characters; " + f"the limit is {MAX_DESCRIPTION_LENGTH}", ) category = _require_str(skill_dir, meta, "category") @@ -139,22 +184,32 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> agents = meta["supported-agents"] if not isinstance(agents, list) or not agents: - raise SkillError(f"{skill_dir}: supported-agents must be a non-empty list") + raise _invalid( + skill_dir, + "meta.supported-agents", + "supported-agents must be a non-empty list", + ) if not all(isinstance(agent, str) for agent in agents): - raise SkillError(f"{skill_dir}: supported-agents entries must be strings") + raise _invalid( + skill_dir, + "meta.supported-agents", + "supported-agents entries must be strings", + ) duplicates = sorted({agent for agent in agents if agents.count(agent) > 1}) if duplicates: - raise SkillError( - f"{skill_dir}: supported-agents lists agent(s) more than once: " - f"{', '.join(duplicates)}" + raise _invalid( + skill_dir, + "meta.supported-agents", + f"supported-agents lists agent(s) more than once: {', '.join(duplicates)}", ) if known_agents is not None: unknown = [a for a in agents if a not in known_agents] if unknown: - raise SkillError( - f"{skill_dir}: supported-agents has unknown agent(s): " - f"{', '.join(unknown)}" + raise _invalid( + skill_dir, + "meta.unknown-agent", + f"supported-agents has unknown agent(s): {', '.join(unknown)}", ) deprecated = ( @@ -166,7 +221,12 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> try: body = body_path.read_text(encoding="utf-8") except UnicodeDecodeError as exc: - raise SkillError(f"{skill_dir}: skill.md is not valid UTF-8: {exc}") from exc + raise _invalid( + skill_dir, + "body.encoding", + f"skill.md is not valid UTF-8: {exc}", + "skill.md", + ) from exc return Skill( name=name, @@ -184,24 +244,30 @@ def _require_str(skill_dir: Path, meta: dict[Any, Any], field: str) -> str: """Return ``meta[field]`` if it is a non-blank string, else fail loudly.""" value = meta[field] if not isinstance(value, str) or not value.strip(): - raise SkillError(f"{skill_dir}: meta.yaml {field} must be a non-empty string") + raise _invalid( + skill_dir, f"meta.{field}", f"meta.yaml {field} must be a non-empty string" + ) return value def _require_version(skill_dir: Path, value: object, field: str) -> str: """Return ``value`` if it is a MAJOR.MINOR.PATCH string, else fail loudly.""" + rule = "meta.version" if field == "version" else "meta.deprecated" if not isinstance(value, str): # An unquoted ``version: 1.10`` is the float 1.1 by the time it gets # here; stringifying it would silently record the wrong version. - raise SkillError( - f"{skill_dir}: meta.yaml {field} must be a string, but YAML read it " + raise _invalid( + skill_dir, + rule, + f"meta.yaml {field} must be a string, but YAML read it " f"as {type(value).__name__} {value!r}; quote it, " - f'e.g. {field.rpartition(".")[2]}: "1.10.0"' + f'e.g. {field.rpartition(".")[2]}: "1.10.0"', ) if not VERSION_RE.fullmatch(value): - raise SkillError( - f"{skill_dir}: meta.yaml {field} {value!r} must be " - "MAJOR.MINOR.PATCH, e.g. 0.1.0" + raise _invalid( + skill_dir, + rule, + f"meta.yaml {field} {value!r} must be MAJOR.MINOR.PATCH, e.g. 0.1.0", ) return value @@ -224,25 +290,30 @@ def _load_deprecation( "replacement); leave it out for a skill that is not deprecated" ) if not isinstance(raw, dict): - raise SkillError(f"{skill_dir}: meta.yaml {shape}") + raise _invalid(skill_dir, "meta.deprecated", f"meta.yaml {shape}") unknown = sorted(str(key) for key in raw if key not in DEPRECATION_FIELDS) if unknown: - raise SkillError( - f"{skill_dir}: meta.yaml deprecated has unknown field(s): " - f"{', '.join(unknown)}; {shape}" + raise _invalid( + skill_dir, + "meta.deprecated", + f"meta.yaml deprecated has unknown field(s): {', '.join(unknown)}; {shape}", ) missing = [field for field in ("since", "reason") if field not in raw] if missing: - raise SkillError( - f"{skill_dir}: meta.yaml deprecated missing fields: {', '.join(missing)}" + raise _invalid( + skill_dir, + "meta.deprecated", + f"meta.yaml deprecated missing fields: {', '.join(missing)}", ) since = _require_version(skill_dir, raw["since"], "deprecated.since") if _version_key(since) > _version_key(version): - raise SkillError( - f"{skill_dir}: meta.yaml deprecated.since {since!r} is later than " + raise _invalid( + skill_dir, + "meta.deprecated", + f"meta.yaml deprecated.since {since!r} is later than " f"the skill's version {version!r}; since is the skill version that " - "first carried the deprecation" + "first carried the deprecation", ) reason = raw["reason"] @@ -252,25 +323,33 @@ def _load_deprecation( # the single-line check below rejects every value that has one. reason = reason.rstrip("\r\n") if not isinstance(reason, str) or not reason.strip(): - raise SkillError( - f"{skill_dir}: meta.yaml deprecated.reason must be a non-empty string" + raise _invalid( + skill_dir, + "meta.deprecated", + "meta.yaml deprecated.reason must be a non-empty string", ) if reason.splitlines() != [reason] or len(reason) > MAX_DESCRIPTION_LENGTH: - raise SkillError( - f"{skill_dir}: meta.yaml deprecated.reason must be a single line of " - f"at most {MAX_DESCRIPTION_LENGTH} characters" + raise _invalid( + skill_dir, + "meta.deprecated", + "meta.yaml deprecated.reason must be a single line of " + f"at most {MAX_DESCRIPTION_LENGTH} characters", ) replacement = raw.get("replacement") if replacement is not None: if not isinstance(replacement, str) or not NAME_RE.fullmatch(replacement): - raise SkillError( - f"{skill_dir}: meta.yaml deprecated.replacement must be a skill " - f"name or null, not {replacement!r}" + raise _invalid( + skill_dir, + "meta.deprecated", + "meta.yaml deprecated.replacement must be a skill " + f"name or null, not {replacement!r}", ) if replacement == name: - raise SkillError( - f"{skill_dir}: meta.yaml deprecated.replacement names the skill itself" + raise _invalid( + skill_dir, + "meta.deprecated", + "meta.yaml deprecated.replacement names the skill itself", ) return Deprecation(since=since, reason=reason, replacement=replacement) @@ -288,44 +367,52 @@ def discover_skills( for child in sorted(root.iterdir()) if child.is_dir() and not child.name.startswith(".") ] - _check_replacements(skills) + errors = replacement_errors(skills) + if errors: + raise errors[0] return skills -def _check_replacements(skills: list[Skill]) -> None: - """Fail unless every deprecated skill's replacement is a current skill - that supports every agent the deprecated one does. +def replacement_errors(skills: list[Skill]) -> list[SkillError]: + """One error per deprecated skill in ``skills`` whose replacement is not a + current skill in ``skills`` that supports every agent the deprecated one + does. A replacement that is missing, deprecated itself, or missing one of those agents would send users to a skill they cannot install or should not adopt. """ by_name = {skill.name: skill for skill in skills} + errors: list[SkillError] = [] for skill in skills: if skill.deprecated is None or skill.deprecated.replacement is None: continue replacement = by_name.get(skill.deprecated.replacement) if replacement is None: - raise SkillError( - f"{skill.path}: meta.yaml deprecated.replacement " + detail = ( + "meta.yaml deprecated.replacement " f"{skill.deprecated.replacement!r} is not a skill in the same " "directory" ) - if replacement.deprecated is not None: - raise SkillError( - f"{skill.path}: meta.yaml deprecated.replacement " + elif replacement.deprecated is not None: + detail = ( + "meta.yaml deprecated.replacement " f"{replacement.name!r} is itself deprecated; name the skill " "that replaces it instead" ) - lacking = [ - agent - for agent in skill.supported_agents - if agent not in replacement.supported_agents - ] - if lacking: - raise SkillError( - f"{skill.path}: meta.yaml deprecated.replacement " + else: + lacking = [ + agent + for agent in skill.supported_agents + if agent not in replacement.supported_agents + ] + if not lacking: + continue + detail = ( + "meta.yaml deprecated.replacement " f"{replacement.name!r} does not support {', '.join(lacking)}, " f"which {skill.name} supports; its users there would have no " "replacement to install" ) + errors.append(_invalid(skill.path, "meta.deprecated-replacement", detail)) + return errors diff --git a/tests/test_authoring.py b/tests/test_authoring.py new file mode 100644 index 0000000..942c1f9 --- /dev/null +++ b/tests/test_authoring.py @@ -0,0 +1,648 @@ +"""``skilldeck new`` and ``skilldeck validate`` (#72). + +A skill is scaffolded into a temporary skills directory, completed, validated, +rendered, installed and removed through the CLI; each validation rule is then +broken on purpose. Checkout mode runs against a minimal skilldeck checkout +built in ``tmp_path`` from this repository's own scripts, so the eval fixture +and generated-output checks run for real. Nothing here touches the network. +""" + +import importlib.util +import json +import re +import shutil +import socket +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from skilldeck import authoring, lint, registry +from skilldeck.adapters import ADAPTERS, ALL_ADAPTERS +from skilldeck.cli import cli +from skilldeck.provenance import canonical_json +from skilldeck.targets import Scope + +ROOT = Path(__file__).resolve().parent.parent +NAME = "widget-review" +DESCRIPTION = "Review pending changes for widget misuse." +SOURCE = "Grounded in the [Widget Guide](https://example.org/widget-guide)." + + +def _runner(): + # Click < 8.2 mixes stderr into stdout unless told not to; 8.2 dropped the + # flag and always captures the streams separately + try: + return CliRunner(mix_stderr=False) # type: ignore[call-arg] + except TypeError: + return CliRunner() + + +def _invoke(*args, code=0): + result = _runner().invoke(cli, [str(arg) for arg in args]) + assert result.exit_code == code, (result.stdout, result.stderr) + return result + + +def _report(*args, code=1): + return json.loads(_invoke("validate", "--json", *args, code=code).stdout) + + +def _rules(report, level=None): + return { + p["rule"] for p in report["problems"] if level is None or p["level"] == level + } + + +def _new(skills_dir, name=NAME, *args): + return _invoke("new", name, "--category", "review", "--dir", skills_dir, *args) + + +def _complete(skill_dir): + """Write the content an author would: every placeholder filled, a cited + source, and a real description.""" + body = (skill_dir / "skill.md").read_text(encoding="utf-8") + body = body.replace(f"{lint.PLACEHOLDER}: ", "") + body = body.replace( + "## What to look for\n\n", f"## What to look for\n\n{SOURCE}\n\n" + ) + (skill_dir / "skill.md").write_text(body, encoding="utf-8", newline="\n") + meta = (skill_dir / "meta.yaml").read_text(encoding="utf-8") + meta = meta.replace(repr(authoring.PLACEHOLDER_DESCRIPTION), DESCRIPTION) + (skill_dir / "meta.yaml").write_text(meta, encoding="utf-8", newline="\n") + assert lint.PLACEHOLDER not in body + meta + + +@pytest.fixture +def skills_dir(tmp_path, monkeypatch): + """An organization's skills directory, outside any checkout.""" + monkeypatch.chdir(tmp_path) + root = tmp_path / "skills" + root.mkdir() + return root + + +@pytest.fixture +def completed(skills_dir): + _new(skills_dir) + _complete(skills_dir / NAME) + return skills_dir / NAME + + +# --- scaffolding --------------------------------------------------------------- + + +def test_new_scaffolds_meta_and_skeleton(skills_dir): + out = _new(skills_dir).stdout + skill_dir = skills_dir / NAME + assert sorted(p.name for p in skill_dir.iterdir()) == ["meta.yaml", "skill.md"] + assert f"created skills/{NAME}/meta.yaml" in out + assert f"skilldeck validate --skills-dir skills {NAME}" in out + # outside a checkout there is nowhere to put an eval fixture + assert not (skills_dir.parent / "evals").exists() + skill = registry.load_skill(skill_dir, set(ADAPTERS)) + assert skill.version == "0.1.0" + assert skill.category == "review" + assert skill.supported_agents == tuple(sorted(ADAPTERS)) + assert skill.body.startswith("# Widget Review\n") + for path in skill_dir.iterdir(): + assert b"\r" not in path.read_bytes() + + +def test_new_takes_description_and_agents(skills_dir): + _new( + skills_dir, + NAME, + "--description", + DESCRIPTION, + "--agent", + "codex", + "--agent", + "claude", + "--agent", + "codex", + ) + skill = registry.load_skill(skills_dir / NAME, set(ADAPTERS)) + assert skill.description == DESCRIPTION + assert skill.supported_agents == ("codex", "claude") + + +def test_generated_skill_passes_every_structural_check(skills_dir): + # Before any content is written, the only problems are the honest + # "incomplete" ones: placeholders remain and no source is cited yet. + _new(skills_dir) + skill = registry.load_skill(skills_dir / NAME, set(ADAPTERS)) + assert lint.structure_problems(skill.name, skill.body) == [] + assert lint.description_problems(skill.name, skill.description) == [] + report = _report("--skills-dir", skills_dir) + assert _rules(report) == {"content.placeholder", "references.cited-source"} + assert _rules(report, "error") == set() + assert report["skills"] == [ + { + "name": NAME, + "path": f"skills/{NAME}", + "checkout": False, + "status": "incomplete", + } + ] + placeholders = { + p["path"]: p for p in report["problems"] if p["rule"] == "content.placeholder" + } + assert set(placeholders) == {f"skills/{NAME}/meta.yaml", f"skills/{NAME}/skill.md"} + + +def test_skeleton_states_no_domain_guidance(): + # everything outside the shared template is a placeholder + text = authoring.SKILL_TEMPLATE + assert not lint.LINK_RE.search(text) + look_for = lint.section(text, "What to look for").strip() + assert look_for.startswith(lint.PLACEHOLDER) + + +@pytest.mark.parametrize( + ("args", "message"), + [ + (["Bad_Name", "--category", "x"], "invalid skill name"), + ([NAME, "--category", ""], "--category must be a single non-empty line"), + ([NAME, "--category", "x", "--description", "No period"], "ending with a"), + ([NAME, "--category", "x", "--agent", "vscode"], "Invalid value for '--agent'"), + ], +) +def test_new_rejects_bad_arguments(skills_dir, args, message): + result = _invoke("new", *args, "--dir", skills_dir, code=2) + assert message in result.stderr + assert not (skills_dir / NAME).exists() + + +def test_new_never_overwrites(completed): + before = (completed / "skill.md").read_text(encoding="utf-8") + result = _invoke("new", NAME, "--category", "x", "--dir", completed.parent, code=2) + assert "already exists" in result.stderr + assert (completed / "skill.md").read_text(encoding="utf-8") == before + + +def test_new_needs_a_directory_outside_a_checkout(skills_dir): + result = _invoke("new", NAME, "--category", "x", code=2) + assert "not inside a skilldeck checkout" in result.stderr + assert "--dir" in result.stderr + + +def test_new_never_writes_into_the_installed_package(tmp_path, monkeypatch): + package = tmp_path / "site-packages" / "skilldeck" + (package / "skills").mkdir(parents=True) + monkeypatch.setattr(registry, "DEFAULT_SKILLS_DIR", package / "skills") + result = _invoke( + "new", NAME, "--category", "x", "--dir", package / "skills", code=2 + ) + assert "inside the installed skilldeck package" in result.stderr + assert not (package / "skills" / NAME).exists() + + +# --- the full lifecycle ----------------------------------------------------------- + + +def test_generate_complete_validate_render_install_remove( + completed, tmp_path, monkeypatch +): + out = _invoke("validate", "--skills-dir", completed.parent).stdout + assert f"{NAME}: ok" in out + assert "1 skill(s): 1 ok, 0 incomplete, 0 invalid" in out + assert _invoke("validate", f"./skills/{NAME}").stdout == out + + # the rest of the CLI reads the bundled skills; point it at these instead + monkeypatch.setattr(registry, "DEFAULT_SKILLS_DIR", completed.parent) + for agent in sorted(ALL_ADAPTERS): + shown = _invoke("show", NAME, "--agent", agent).stdout + assert "# Widget Review" in shown, agent + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + _invoke("install", NAME, "--agent", "all", "--agent", "copilot-prompt") + skill = registry.load_skill(completed, set(ADAPTERS)) + installed = [ + ALL_ADAPTERS[agent].destination(skill, Scope.PROJECT) + for agent in [*ADAPTERS, "copilot-prompt"] + ] + assert all(path.is_file() for path in installed) + status = _invoke("status", "--agent", "all").stdout + assert status.count(f"{NAME} 0.1.0 up to date") == len(ADAPTERS) + _invoke("uninstall", NAME, "--agent", "all", "--agent", "copilot-prompt") + assert not any(path.exists() for path in installed) + + +# --- each rule, broken on purpose ------------------------------------------------- + + +def _edit(path, old, new): + text = path.read_text(encoding="utf-8") + assert old in text + path.write_text(text.replace(old, new), encoding="utf-8", newline="\n") + + +def _problem(report, rule): + (problem,) = [p for p in report["problems"] if p["rule"] == rule] + return problem + + +def test_malformed_metadata(completed): + _edit(completed / "meta.yaml", "version: 0.1.0", "version: 1.10") + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "meta.version") + assert problem["path"] == f"skills/{NAME}/meta.yaml" + assert problem["level"] == "error" + assert "float 1.1" in problem["message"] + assert "quote" in problem["remediation"] + assert report["skills"][0]["status"] == "invalid" + # skill.md is still checked, but nothing that needs the loaded skill + assert _rules(report) == {"meta.version"} + assert [s["check"] for s in report["skipped"]][0] == ( + f"rendering and catalog entry of {NAME}" + ) + + +def test_unparseable_metadata(completed): + (completed / "meta.yaml").write_text("name: [unclosed\n", encoding="utf-8") + problem = _problem(_report("--skills-dir", completed.parent), "meta.syntax") + assert problem["path"] == f"skills/{NAME}/meta.yaml" + + +def test_unsupported_agent(completed): + _edit(completed / "meta.yaml", " - kiro\n", " - kiro\n - vscode\n") + problem = _problem(_report("--skills-dir", completed.parent), "meta.unknown-agent") + assert "vscode" in problem["message"] + assert "claude, codex, copilot, cursor and kiro" in problem["remediation"] + + +def test_legacy_adapter_names_are_not_agents(completed): + _edit(completed / "meta.yaml", " - kiro\n", " - kiro\n - cursor-rule\n") + report = _report("--skills-dir", completed.parent) + assert "follow their agent" in _problem(report, "meta.unknown-agent")["remediation"] + + +def test_missing_reference(completed): + _edit(completed / "skill.md", SOURCE, "Grounded in the Widget Guide.") + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "references.cited-source") + assert problem["path"] == f"skills/{NAME}/skill.md" + assert problem["level"] == "incomplete" + assert report["skills"][0]["status"] == "incomplete" + + +def test_superseded_reference(completed): + _edit(completed / "skill.md", SOURCE, SOURCE + " See ASVS v4.0.3.") + problem = _problem( + _report("--skills-dir", completed.parent), "references.superseded" + ) + assert problem["line"] and "ASVS 5.0" in problem["message"] + + +def test_missing_section(completed): + _edit(completed / "skill.md", "\n## Scope\n", "\n## Where to look\n") + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "structure.section") + assert "## Scope" in problem["message"] + assert problem["path"] == f"skills/{NAME}/skill.md" + + +def test_reworded_rubric(completed): + _edit(completed / "skill.md", "readily triggered", "easily triggered") + problem = _problem( + _report("--skills-dir", completed.parent), "structure.severity-rubric" + ) + lines = (completed / "skill.md").read_text(encoding="utf-8").split("\n") + assert problem["line"] == lines.index("## Output") + 1 + + +def test_heading_must_spell_the_name(completed): + _edit(completed / "skill.md", "# Widget Review\n", "# Gadget Review\n") + problem = _problem(_report("--skills-dir", completed.parent), "structure.heading") + assert problem["line"] == 1 + + +def test_leftover_placeholder_is_located(completed): + _edit(completed / "skill.md", SOURCE, f"{SOURCE}\n\n{lint.PLACEHOLDER}: more.") + problem = _problem(_report("--skills-dir", completed.parent), "content.placeholder") + text = (completed / "skill.md").read_text(encoding="utf-8") + assert problem["line"] == text.split(lint.PLACEHOLDER)[0].count("\n") + 1 + + +def test_unexpected_file(completed): + (completed / "notes.txt").write_text("scratch\n", encoding="utf-8") + problem = _problem( + _report("--skills-dir", completed.parent), "skill.unexpected-file" + ) + assert problem["path"] == f"skills/{NAME}/notes.txt" + + +# YAML must quote it (": "), and Cursor's reader can't undo either quoting +QUOTED = 'Review "key: value" pairs that aren\'t escaped.' + + +def test_render_failure_names_the_adapter(skills_dir): + _new(skills_dir, NAME, "--description", QUOTED) + _complete(skills_dir / NAME) + problem = _problem(_report("--skills-dir", skills_dir), "render.failed") + assert "cursor-rule" in problem["message"] + assert problem["path"] == f"skills/{NAME}/meta.yaml" + + +def test_catalog_entry_failure(completed, monkeypatch): + def broken(skill): + raise OSError("disk on fire") + + monkeypatch.setattr(authoring, "skill_entry", broken) + problem = _problem(_report("--skills-dir", completed.parent), "catalog.entry") + assert "disk on fire" in problem["message"] + + +def test_deprecated_replacement_must_exist(completed): + meta = completed / "meta.yaml" + meta.write_text( + meta.read_text(encoding="utf-8") + + "deprecated:\n since: 0.1.0\n replacement: gone\n reason: Folded.\n", + encoding="utf-8", + ) + problem = _problem( + _report("--skills-dir", completed.parent), "meta.deprecated-replacement" + ) + assert "'gone' is not a skill" in problem["message"] + + +def test_missing_skill_md(completed): + (completed / "skill.md").unlink() + report = _report("--skills-dir", completed.parent) + assert _problem(report, "body.missing")["path"] == f"skills/{NAME}/skill.md" + + +# --- output -------------------------------------------------------------------- + + +def test_human_output_names_file_rule_and_fix(completed): + _edit(completed / "skill.md", "\n## Scope\n", "\n## Where to look\n") + out = _invoke("validate", "--skills-dir", completed.parent, code=1).stdout + lines = out.splitlines() + index = next(i for i, line in enumerate(lines) if "[structure.section]" in line) + assert lines[index].startswith(f"skills/{NAME}/skill.md: error [structure.section]") + assert lines[index + 1].startswith(" fix: add the section") + assert f"{NAME}: invalid" in out + assert out.rstrip().endswith("1 skill(s): 0 ok, 0 incomplete, 1 invalid") + + +def test_json_is_deterministic_and_sorted(skills_dir): + _new(skills_dir, "b-review") + _new(skills_dir, "a-review") + _edit(skills_dir / "a-review" / "meta.yaml", "version: 0.1.0", "version: 1.10") + first = _invoke("validate", "--json", "--skills-dir", skills_dir, code=1) + second = _invoke("validate", "--json", "--skills-dir", skills_dir, code=1) + assert first.stdout_bytes == second.stdout_bytes + assert first.stdout_bytes == canonical_json(json.loads(first.stdout)).encode() + assert b"\r" not in first.stdout_bytes + data = json.loads(first.stdout) + assert data["schema_version"] == authoring.REPORT_SCHEMA_VERSION + assert data["ok"] is False + assert [s["name"] for s in data["skills"]] == ["a-review", "b-review"] + keys = [ + (p["skill"], p["path"], p["line"] or 0, p["rule"]) for p in data["problems"] + ] + assert keys == sorted(keys) + for problem in data["problems"]: + assert set(problem) == { + "skill", + "path", + "line", + "rule", + "level", + "message", + "remediation", + } + assert problem["rule"] in lint.RULES + assert problem["remediation"] + + +def test_every_rule_is_documented(): + doc = (ROOT / "docs" / "authoring-skills.md").read_text(encoding="utf-8") + documented = dict( + re.findall(r"^\| `([a-z]+\.[a-z-]+)` \| (error|incomplete) \|", doc, re.M) + ) + assert documented == {rule: spec.level for rule, spec in lint.RULES.items()} + + +# --- choosing what to validate ------------------------------------------------------ + + +def test_validate_needs_a_skills_directory_outside_a_checkout(skills_dir): + result = _invoke("validate", code=2) + assert "--skills-dir" in result.stderr + result = _invoke("validate", NAME, code=2) + assert "not inside a skilldeck checkout" in result.stderr + + +def test_validate_rejects_an_unknown_skill(completed): + result = _invoke("validate", "--skills-dir", completed.parent, "nope", code=2) + assert "no skill 'nope' in skills" in result.stderr + + +def test_validate_suggests_a_path_for_a_local_directory(completed, monkeypatch): + other = completed.parent.parent / "elsewhere" + (other / "local").mkdir(parents=True) + monkeypatch.chdir(other) + result = _invoke("validate", "--skills-dir", completed.parent, "local", code=2) + assert "pass ./local" in result.stderr + + +# --- in a skilldeck checkout ---------------------------------------------------------- + +FINDING_OUTPUT = textwrap.dedent( + f"""\ + # Finding output format + + Every review skill (`{NAME}`) reports findings in one shape. + + ## Fields + + | Skill | `classifier` is… | Example | + | --- | --- | --- | + | `{NAME}` | the widget concern | `Widget misuse` | + """ +) + + +def _load(path): + """Import a checkout script by path, as ``skilldeck validate`` does.""" + saved = sys.path[:] + name = f"_test_{path.stem}_{abs(hash(path))}" + spec = importlib.util.spec_from_file_location(name, path) + module = importlib.util.module_from_spec(spec) + # dataclasses look their module up in sys.modules + sys.modules[name] = module + try: + spec.loader.exec_module(module) + finally: + sys.path[:] = saved + return module + + +@pytest.fixture +def checkout(tmp_path, monkeypatch): + """A minimal skilldeck checkout: this repository's generator and eval + runner, no skills, and a finding-output doc that lists widget-review.""" + root = tmp_path / "skilldeck" + for relative in ( + "scripts/build_plugin.py", + "scripts/_pyproject.py", + "evals/run_evals.py", + ): + (root / relative).parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(ROOT / relative, root / relative) + (root / "src" / "skilldeck" / "skills").mkdir(parents=True) + (root / "evals" / "fixtures").mkdir() + (root / "docs").mkdir() + (root / "docs" / "finding-output.md").write_text(FINDING_OUTPUT, encoding="utf-8") + (root / "pyproject.toml").write_text( + '[project]\nname = "skilldeck"\nversion = "0.3.0"\n', encoding="utf-8" + ) + monkeypatch.chdir(root) + # the copied build_plugin.py imports its own _pyproject; keep that copy + # out of the other tests' way + monkeypatch.delitem(sys.modules, "_pyproject", raising=False) + return root + + +def _plant(fixture_dir): + """Turn the scaffolded fixture into a planted one.""" + for part in ("base", "change"): + (fixture_dir / part / "README.md").unlink() + (fixture_dir / "base" / "widgets.py").write_text( + "def spin(widget):\n return widget.spin(speed=1)\n", encoding="utf-8" + ) + (fixture_dir / "change" / "widgets.py").write_text( + "def spin(widget):\n return widget.spin(speed=10**9)\n", encoding="utf-8" + ) + (fixture_dir / "expected.yaml").write_text( + f"skill: {NAME}\nplants:\n - file: widgets.py\n keywords: [overspeed]\n" + "max-findings: 2\n", + encoding="utf-8", + ) + + +def _regenerate(root): + build = _load(root / "scripts" / "build_plugin.py") + build.write(build.generate(root, root / "src" / "skilldeck" / "skills"), root) + + +def test_checkout_detection(checkout): + assert authoring.find_checkout(checkout / "src") == authoring.Checkout( + checkout.resolve() + ) + assert authoring.checkout_of(checkout / "src" / "skilldeck" / "skills") + assert authoring.checkout_of(checkout / "docs") is None + assert authoring.find_checkout(checkout.parent) is None + + +def test_checkout_lifecycle(checkout, capsys): + out = _invoke( + "new", NAME, "--category", "review", "--description", DESCRIPTION + ).stdout + skill_dir = checkout / "src" / "skilldeck" / "skills" / NAME + fixture_dir = checkout / "evals" / "fixtures" / NAME + assert f"created evals/fixtures/{NAME}/expected.yaml" in out + assert "scripts/build_plugin.py" in out + + # the fixture skeleton is a valid (clean-diff) fixture, so committing it + # breaks nothing, but validate still reports what is left to write + runner = _load(checkout / "evals" / "run_evals.py") + fixture = runner.load_fixture(fixture_dir) + assert fixture.skill == NAME and fixture.plants == () + assert runner.fixture_layout_problems(fixture) == [] + + report = _report() + assert report["skills"][0]["checkout"] is True + assert report["skills"][0]["status"] == "incomplete" + assert _rules(report, "incomplete") == { + "content.placeholder", + "references.cited-source", + "eval.fixture-missing", + } + # nothing is generated for the new skill yet + stale = [p for p in report["problems"] if p["rule"] == "generated.stale"] + assert f"claude-plugin/skills/{NAME}/SKILL.md" in {p["path"] for p in stale} + assert all(p["skill"] is None and p["level"] == "error" for p in stale) + assert _rules(report, "error") == {"generated.stale"} + + _complete(skill_dir) + _plant(fixture_dir) + _regenerate(checkout) + capsys.readouterr() + clean = _invoke("validate", NAME).stdout + assert f"{NAME}: ok" in clean + + # an edit without regenerating leaves the generated tree stale + _edit(skill_dir / "skill.md", SOURCE, SOURCE + " Also see the FAQ.") + report = _report(NAME) + assert _rules(report) == {"generated.stale"} + assert {(p["path"], p["message"]) for p in report["problems"]} >= { + (f"claude-plugin/skills/{NAME}/SKILL.md", "generated file is out of date"), + ("src/skilldeck/_content_manifest.json", "generated file is out of date"), + } + assert "scripts/build_plugin.py" in report["problems"][0]["remediation"] + + +def test_validate_is_offline(checkout, monkeypatch): + # no network and no subprocess (no git, no agent) for any check + _invoke("new", NAME, "--category", "review") + + def refuse(*args, **kwargs): + raise AssertionError("validate tried to reach outside the process") + + monkeypatch.setattr(socket, "socket", refuse) + monkeypatch.setattr(socket, "create_connection", refuse) + monkeypatch.setattr(subprocess, "run", refuse) + monkeypatch.setattr(subprocess, "Popen", refuse) + report = _report(NAME) + assert "generated.stale" in _rules(report) + assert "eval.fixture-missing" in _rules(report) + + +def test_checkout_missing_eval_fixture(checkout): + _invoke("new", NAME, "--category", "review", "--no-eval-fixture") + assert not (checkout / "evals" / "fixtures" / NAME).exists() + problem = _problem(_report(NAME), "eval.fixture-missing") + assert problem["message"] == f"no eval fixture exercises {NAME}" + assert problem["path"] == f"evals/fixtures/{NAME}" + assert f"add evals/fixtures/{NAME}/" in problem["remediation"] + + +def test_checkout_invalid_eval_fixture(checkout): + _invoke("new", NAME, "--category", "review") + fixture_dir = checkout / "evals" / "fixtures" / NAME + _edit(fixture_dir / "expected.yaml", "max-findings: 0", "max-findings: 0\nextra: 1") + problem = _problem(_report(NAME), "eval.fixture-invalid") + assert problem["path"] == f"evals/fixtures/{NAME}/expected.yaml" + assert "unknown key(s) ['extra']" in problem["message"] + + +def test_checkout_new_refuses_an_existing_fixture(checkout): + (checkout / "evals" / "fixtures" / NAME).mkdir() + result = _invoke("new", NAME, "--category", "review", code=2) + assert "--no-eval-fixture" in result.stderr + assert not (checkout / "src" / "skilldeck" / "skills" / NAME).exists() + + +def test_checkout_skill_missing_from_the_finding_output_doc(checkout): + _invoke("new", "other-review", "--category", "review") + problem = _problem(_report("other-review"), "docs.finding-output") + assert problem["path"] == "docs/finding-output.md" + + +def test_bundled_skills_validate_clean(): + # the real checkout: every bundled skill passes every check, placeholders + # and generated output included (the same rules the tests above break) + skills_dir = ROOT / "src" / "skilldeck" / "skills" + report = authoring.validate(authoring.skill_dirs(skills_dir), base=ROOT) + assert report.problems == [], authoring.format_report(report) + assert report.skipped == [] + assert {skill.status for skill in report.skills} == {"ok"} + assert all(skill.checkout for skill in report.skills) diff --git a/tests/test_skill_citations.py b/tests/test_skill_citations.py index b43a31b..48f7c5c 100644 --- a/tests/test_skill_citations.py +++ b/tests/test_skill_citations.py @@ -4,67 +4,66 @@ cited in the body. These checks keep those citations current (#102): every skill links a source, none cites a superseded edition of a standard, none links a documentation path that only survives as a redirect, and the ASVS requirement -IDs security-review cites sit under the chapter they belong to. +IDs security-review cites sit under the chapter they belong to. The general +rules live in ``skilldeck.lint``, which ``skilldeck validate`` reports too. """ import re import pytest +from skilldeck import lint from skilldeck.adapters import ADAPTERS from skilldeck.registry import discover_skills SKILLS = discover_skills(known_agents=set(ADAPTERS)) -LINK_RE = re.compile(r"\]\((https://[^)\s]+)\)") - -# pattern -> what to cite instead -SUPERSEDED = { - r"\bA\d{2}:2021\b|/Top10/A\d{2}_2021-": "OWASP Top 10:2025 (owasp.org/Top10/2025/)", - r"\bASVS\s*v?4\.": "ASVS 5.0", - # the 2026 edition renumbered the entries, so a 2025 ID names a different risk - r"\bLLM\d{2}:2025\b": "OWASP Top 10 for LLM Applications 2026 (LLMxx:2026)", -} - -# old doc paths that only work through redirects -> the canonical form -STALE_URL_PREFIXES = { - "https://docs.gitlab.com/ee/": "https://docs.gitlab.com//", - "https://docs.github.com/en/actions/security-for-github-actions/": ( - "https://docs.github.com/en/actions/reference/security/..." - ), - "https://docs.github.com/en/actions/security-guides/": ( - "https://docs.github.com/en/actions/reference/security/..." - ), -} + + +def _problems(skill, rule): + return [ + p.message + for p in lint.reference_problems(skill.name, skill.body) + if p.rule == rule + ] @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) def test_skill_cites_at_least_one_source(skill): - assert LINK_RE.search(skill.body), ( + assert not _problems(skill, "references.cited-source"), ( f"{skill.name}/skill.md links no authoritative source" ) @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) def test_skill_cites_no_superseded_standard(skill): - stale = [ - f"{m.group(0)!r} (cite {current})" - for pattern, current in SUPERSEDED.items() - for m in re.finditer(pattern, skill.body) - ] + stale = _problems(skill, "references.superseded") assert not stale, f"{skill.name}/skill.md: " + "; ".join(stale) @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) def test_skill_links_no_redirect_only_doc_paths(skill): - stale = [ - f"{url} (use {canonical})" - for url in LINK_RE.findall(skill.body) - for prefix, canonical in STALE_URL_PREFIXES.items() - if url.startswith(prefix) - ] + stale = _problems(skill, "references.redirect-url") assert not stale, f"{skill.name}/skill.md: " + "; ".join(stale) +@pytest.mark.parametrize( + ("text", "rule"), + [ + ("See the OWASP cheat sheets.", "references.cited-source"), + ( + "[A03:2021](https://owasp.org/Top10/A03_2021-Injection/)", + "references.superseded", + ), + ("[ASVS](https://owasp.org/asvs) ASVS v4.0.3", "references.superseded"), + ("[LLM01:2025](https://genai.owasp.org/)", "references.superseded"), + ("[docs](https://docs.gitlab.com/ee/ci/)", "references.redirect-url"), + ], +) +def test_reference_rules_catch_what_they_are_for(text, rule): + problems = lint.reference_problems("x", text) + assert rule in {p.rule for p in problems} + + def test_security_review_asvs_ids_sit_under_their_chapter(): body = next(s for s in SKILLS if s.name == "security-review").body bullets = re.findall( diff --git a/tests/test_skill_structure.py b/tests/test_skill_structure.py index 4b8f34c..011ee77 100644 --- a/tests/test_skill_structure.py +++ b/tests/test_skill_structure.py @@ -2,11 +2,13 @@ The Scope/Output boilerplate is hand-maintained across every skill and drifts (the ``logging`` skill once shipped without a Scope section, and later without -an ``## Output`` heading). These tests pin the structural elements every skill -must carry, asserting on stable phrases or loose patterns rather than exact -wording so editing skills stays low-friction. The one exception is the shared -severity rubric (#103), which ``docs/finding-output.md`` defines once and each -skill inlines word for word, so the tests compare it to the doc. +an ``## Output`` heading). The rules that pin the structural elements every +skill must carry live in ``skilldeck.lint``, where ``skilldeck validate`` +reports them too; these tests apply them to every bundled skill. They assert +on stable phrases or loose patterns rather than exact wording so editing +skills stays low-friction. The one exception is the shared severity rubric +(#103), which ``docs/finding-output.md`` defines once and each skill inlines +word for word; ``skilldeck.lint.SEVERITY_RUBRIC`` must match the doc. """ import re @@ -14,6 +16,7 @@ import pytest +from skilldeck import lint from skilldeck.adapters import ADAPTERS from skilldeck.registry import discover_skills @@ -24,96 +27,45 @@ # security-exploitable, data-loss, or outage-causing findings. CAPPED_AT_HIGH = {"code-smells", "test-review"} -# level-2 heading -> why it must be present -REQUIRED_HEADINGS = { - "Scope": "a Scope section saying what to review", - "Output": "an Output section with the finding format", -} -# phrase (matched against whitespace-normalized text) -> why it must be present -REQUIRED_PHRASES = { - "uncommitted": "diff determination covering uncommitted/untracked changes", - "For example:": "a worked example finding", - "Verify before reporting": "the verify-before-reporting instruction", - "Open the report with one line": "the one-line report header instruction", -} - -# regex (matched against whitespace-normalized text) -> why it must match -SCOPE_PATTERNS = { - r"`git fetch`": "a fetch so the diff uses an up-to-date remote base", - r"`git diff origin/\.\.\.HEAD`": "a three-dot diff against origin/", - r"`git (?:ls-files --others --exclude-standard|status --porcelain)`": ( - "a command that lists untracked files" - ), -} -OUTPUT_PATTERNS = { - r"shared severity rubric": "a reference to the shared severity rubric", - r"more than ~\d+ survive.*summarize the rest": "the findings-cap sentence", - r"`Reviewed origin/\w+\.\.\.HEAD \(": "a three-dot range in the header example", -} -NOTHING_IN_SCOPE = r"say so and stop" - - -def _normalize(text): - return " ".join(text.split()) - - -def _section(body, heading): - """The text under ``## heading`` up to the next level-2 heading.""" - match = re.search(rf"^## {re.escape(heading)}\n(.*?)(?=^## |\Z)", body, re.M | re.S) - return match.group(1) if match else "" + +def _explain(problems): + return "; ".join(f"[{p.rule}] {p.message}" for p in problems) def _doc_rubric(): """The one-paragraph rubric that docs/finding-output.md says skills inline.""" - rubric = _section(FINDING_OUTPUT_DOC.read_text(encoding="utf-8"), "Severity rubric") + rubric = lint.section( + FINDING_OUTPUT_DOC.read_text(encoding="utf-8"), "Severity rubric" + ) quoted = [line[2:] for line in rubric.splitlines() if line.startswith("> ")] assert quoted, "docs/finding-output.md lost its quoted rubric paragraph" - return _normalize("\n".join(quoted)) + return lint.normalize("\n".join(quoted)) -@pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_has_required_structure(skill): - body = _normalize(skill.body) - missing = [ - f"'## {heading}' ({why})" - for heading, why in REQUIRED_HEADINGS.items() - if not re.search(rf"^## {heading}$", skill.body, re.M) - ] + [ - f"{phrase!r} ({why})" - for phrase, why in REQUIRED_PHRASES.items() - if phrase not in body - ] - assert not missing, f"{skill.name}/skill.md is missing: " + "; ".join(missing) +def test_package_rubric_is_the_doc_rubric(): + # skilldeck validate and skilldeck new use the package copy; the doc is + # where people read it, so the two must never drift apart + assert lint.normalize(lint.SEVERITY_RUBRIC) == _doc_rubric() @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_scope_and_output_carry_the_shared_pieces(skill): - scope = _normalize(_section(skill.body, "Scope")) - output = _normalize(_section(skill.body, "Output")) - missing = [ - f"{why} (/{pattern}/)" - for patterns, text in ((SCOPE_PATTERNS, scope), (OUTPUT_PATTERNS, output)) - for pattern, why in patterns.items() - if not re.search(pattern, text) - ] - if not re.search(NOTHING_IN_SCOPE, _normalize(skill.body), re.I): - missing.append("a line for a change that touches nothing in scope") - assert not missing, f"{skill.name}/skill.md is missing: " + "; ".join(missing) +def test_skill_follows_the_structural_template(skill): + problems = lint.structure_problems(skill.name, skill.body) + assert not problems, f"{skill.name}/skill.md: " + _explain(problems) @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_inlines_the_shared_severity_rubric(skill): - assert _doc_rubric() in _normalize(_section(skill.body, "Output")), ( - f"{skill.name}/skill.md: the Output section must inline the severity " - "rubric paragraph from docs/finding-output.md word for word" - ) +def test_skill_description_is_a_single_sentence_line(skill): + problems = lint.description_problems(skill.name, skill.description) + assert not problems, f"{skill.name}/meta.yaml: " + _explain(problems) @pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_uses_three_dot_ranges_only(skill): - # `main..feature`, `..HEAD`; not `...`, and not a `../` path - two_dot = re.findall(r"[\w/<>-]*[\w>]\.\.(?!\.)[\w<][\w/<>-]*", skill.body) - assert not two_dot, f"{skill.name}/skill.md uses a two-dot range: {two_dot}" +def test_skill_has_no_placeholder_left(skill): + meta = (skill.path / "meta.yaml").read_text(encoding="utf-8") + problems = lint.placeholder_problems(skill.body, "skill.md") + problems += lint.placeholder_problems(meta, "meta.yaml") + assert not problems, f"{skill.name}: " + _explain(problems) def test_capped_skills_exist(): @@ -126,7 +78,7 @@ def test_capped_skills_exist(): ) def test_non_security_skills_never_rate_critical(skill): # The inlined rubric defines critical; nothing else may use it. - rest = _normalize(skill.body).replace(_doc_rubric(), "") + rest = lint.normalize(skill.body).replace(_doc_rubric(), "") assert not re.search(r"\bcritical\b", rest, re.I), ( f"{skill.name}/skill.md mentions critical outside the shared rubric" ) @@ -135,35 +87,46 @@ def test_non_security_skills_never_rate_critical(skill): def test_finding_output_doc_lists_every_skill(): doc = FINDING_OUTPUT_DOC.read_text(encoding="utf-8") - intro = doc.split("\n## ", 1)[0] - table = _section(doc, "Fields") - missing = [ - s.name - for s in SKILLS - if f"`{s.name}`" not in intro or f"| `{s.name}` |" not in table - ] - assert not missing, f"docs/finding-output.md does not list: {missing}" - - -@pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_heading_matches_name(skill): - first_line = skill.body.splitlines()[0] - assert first_line.startswith("# "), f"{skill.name}: body must open with a heading" - slug = re.sub(r"[^a-z0-9]+", "-", first_line[2:].strip().lower()).strip("-") - assert slug == skill.name, ( - f"{skill.name}: heading {first_line!r} does not match the skill name" - ) - - -@pytest.mark.parametrize("skill", SKILLS, ids=lambda s: s.name) -def test_skill_description_is_a_single_sentence_line(skill): - assert "\n" not in skill.description, f"{skill.name}: description must be one line" - assert skill.description.endswith("."), ( - f"{skill.name}: description should end with a period" - ) + problems = [p for s in SKILLS for p in lint.finding_output_problems(doc, s.name)] + assert not problems, _explain(problems) def test_all_bundled_skills_are_covered(): # If discovery ever silently returns nothing, every parametrized test above # would pass vacuously. assert len(SKILLS) >= 7 + + +# --- the rules catch what they are for ------------------------------------------ + +_GOOD = SKILLS[0] + + +def _rules(body, name=_GOOD.name): + return {p.rule for p in lint.structure_problems(name, body)} + + +def test_rules_catch_a_missing_section(): + body = _GOOD.body.replace("\n## Scope\n", "\n## Where to look\n") + assert "structure.section" in _rules(body) + + +def test_rules_catch_a_reworded_rubric(): + body = _GOOD.body.replace("readily triggered", "easily triggered") + assert _rules(body) == {"structure.severity-rubric"} + + +def test_rules_catch_a_heading_that_does_not_spell_the_name(): + assert _rules(_GOOD.body, name="another-name") == {"structure.heading"} + + +def test_rules_catch_a_two_dot_range(): + body = _GOOD.body + "\nCompare `main..feature` first.\n" + (problem,) = lint.structure_problems(_GOOD.name, body) + assert problem.rule == "structure.two-dot-range" + assert problem.line == body.count("\n") + + +def test_rules_read_crlf_bodies(): + body = _GOOD.body.replace("\n", "\r\n") + assert not lint.structure_problems(_GOOD.name, body) From 037d24f07669603901ba0a586d43bf9fcc8cd8c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 03:18:31 +0000 Subject: [PATCH 2/2] Harden skilldeck validate: trust, symlinks, YAML lines, fixture checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings on the skill author commands (#72): - validate no longer runs code from the tree it checks. The eval-fixture and generated-output checks import a checkout's evals/run_evals.py and scripts/build_plugin.py, so they now run only when that checkout's src/skilldeck is the running package (_trusted_checkout); for any other tree that looks like a checkout (a fork, an archive, a checkout checked by an installed skilldeck) they are reported as skipped with the `uv run --extra dev skilldeck validate` command to run inside it. The text-only finding-output check still applies. A regression test builds a mimic tree whose scripts write a sentinel and validates it through cwd, --skills-dir, a path argument and a cwd inside the skills directory. - Symlinks in a skill directory are reported (new rule skill.symlink, from lint.bundle_problems, which also owns skill.unexpected-file) and never read, so a link's target path and contents cannot reach the report; the other skill file is still checked (registry.check_meta validates meta.yaml alone). Paths are shown with os.path.abspath, so a symlinked skills directory keeps the name the user gave. - A YAML syntax error in meta.yaml is reported on its line, with a one-line message and no PyYAML excerpt of the file; the registry's own message is unchanged. - validate now also checks what the fixture tests did: plant keywords that echo the planted code (eval.keyword-echo) and a clean-diff fixture's tolerance (eval.clean-tolerance), through run_evals helpers that tests/test_eval_fixtures.py shares. `new`'s checkout steps and the authoring walkthrough now name the SAMPLE_REPORTS entry and running pytest before pushing. - Nits: restore the 
 escape text in a registry comment; reject empty validate targets; "no errors in the skill" in the incomplete status line; shlex-quote the --skills-dir hint; test that removing each required structural piece fires its rule (and make the heading lookup independent of REQUIRED_HEADINGS); drop the now-unused report notes; rewrap CLAUDE.md and document the trust rule there and in docs/authoring-skills.md. Part of #72 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HtiCGzpikMrkDYBkfQG5CX --- CHANGELOG.md | 21 ++- CLAUDE.md | 42 +++--- docs/authoring-skills.md | 50 +++++-- evals/run_evals.py | 40 ++++++ src/skilldeck/authoring.py | 240 +++++++++++++++++++++------------- src/skilldeck/cli.py | 15 ++- src/skilldeck/lint.py | 66 +++++++++- src/skilldeck/registry.py | 95 +++++++++++--- tests/test_authoring.py | 158 +++++++++++++++++++++- tests/test_eval_fixtures.py | 35 +++-- tests/test_skill_structure.py | 62 ++++++++- 11 files changed, 659 insertions(+), 165 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de0b0ec..fca8b2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -526,13 +526,18 @@ All notable changes to this project are documented here. The format is based on checkout it also scaffolds `evals/fixtures/NAME/` (skip with `--no-eval-fixture`). `skilldeck validate [NAME|PATH]... [--skills-dir] [--json]` checks skills offline: metadata, structure, cited sources, - leftover placeholders, stray files, rendering by every adapter (legacy - formats included) and the catalog entry; in a checkout also the eval - fixtures (loaded through `evals/run_evals.py`), the skill's row in + leftover placeholders, stray files and symlinks (reported, never + followed), rendering by every adapter (legacy formats included) and the + catalog entry; in a checkout also the eval fixtures (loaded through + `evals/run_evals.py`: layout, keywords that echo the planted code, a + clean-diff fixture's tolerance), the skill's row in `docs/finding-output.md`, and generated-output freshness - (`scripts/build_plugin.py --check`, in process). Each problem names the - file and line, a rule id, and a fix; `--json` is deterministic, and the - exit status is 0 only when clean. A fresh skeleton passes every metadata + (`scripts/build_plugin.py --check`, in process). It never runs code from + the tree it checks: the checks that import a checkout's scripts run only + when that checkout's `src/skilldeck` is the running skilldeck, and are + reported as skipped otherwise. Each problem names the file and line, a + rule id, and a fix; `--json` is deterministic, and the exit status is 0 + only when clean. A fresh skeleton passes every metadata and structure check and is rated `incomplete` (not `invalid`) until its placeholders, sources and eval fixture are written. Outside a checkout both commands need an explicit directory (`--dir`, `--skills-dir`), for @@ -541,7 +546,9 @@ All notable changes to this project are documented here. The format is based on and documents the review path for official and organization skills. The structure and citation rules moved from the tests into `skilldeck.lint`, which the tests and `validate` share, and the registry's errors carry the - rule they break. + rule they break (and, for a YAML syntax error, its line). The fixture + keyword-echo and clean-tolerance checks moved into `evals/run_evals.py` + helpers that the fixture tests and `validate` share. - Agent compatibility matrix, `docs/compatibility.md` (#79), linked from the README and `docs/adapters.md`. It lists every adapter, including the `copilot-prompt`, `cursor-rule` and `kiro-steering` legacy adapters, with: diff --git a/CLAUDE.md b/CLAUDE.md index d1b377d..094beaf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,19 +53,22 @@ by `--agent all`); `skilldeck migrate` moves old-format installs to `SKILL.md`. - `registry.py` — discovers and validates skills, including the optional `deprecated` metadata; each `SkillError` for a malformed skill carries its `validate` rule id - - `lint.py` — the one home of the skill rules (structural template, severity - rubric copy, citation hygiene, placeholders) and the `RULES` table of every - `validate` rule id, level and remediation; `tests/test_skill_structure.py` - and `test_skill_citations.py` apply the same functions to the bundled skills. - Add a rule here, and to the rules table in `docs/authoring-skills.md` (a - test compares them) - - `authoring.py` — `skilldeck new` (scaffold, `TODO(author)` placeholders, no - domain guidance) and `skilldeck validate` (per-skill checks plus, in a + - `lint.py` — the one home of the skill rules (skill-directory bundle, + structural template, severity rubric copy, citation hygiene, placeholders) + and the `RULES` table of every `validate` rule id, level and remediation; + `tests/test_skill_structure.py` and `test_skill_citations.py` apply the + same functions to the bundled skills. Add a rule here and to the rules + table in `docs/authoring-skills.md` (a test compares them) + - `authoring.py` — `skilldeck new` (scaffold, `TODO(author)` placeholders, + no domain guidance) and `skilldeck validate` (per-skill checks plus, in a checkout's `src/skilldeck/skills`, eval fixtures, `docs/finding-output.md` - and generated-output freshness via the checkout's own `evals/run_evals.py` - and `scripts/build_plugin.py`, imported in process). Outside a checkout it - needs an explicit `--dir`/`--skills-dir` and never writes into the - installed package + and generated-output freshness via the checkout's own + `evals/run_evals.py` and `scripts/build_plugin.py`, imported in process). + Outside a checkout it needs an explicit `--dir`/`--skills-dir` and never + writes into the installed package. `validate` never runs code from the + tree it checks: the script-importing checks run only when the checkout's + `src/skilldeck` is the running package (`_trusted_checkout`), else they + are reported as skipped; symlinks in a skill are reported, never read - `catalog.py` + `catalog.schema.json` — the public, schema-versioned `skilldeck catalog --json` contract (the schema ships in the wheel); change it only per the compatibility rules in `docs/catalog.md` (bump @@ -130,15 +133,16 @@ by `--agent all`); `skilldeck migrate` moves old-format installs to `SKILL.md`. *unverified*. - New skills start from `skilldeck new`, follow the structural template (the `skilldeck.lint` rules, which `skilldeck validate` reports and - `tests/test_skill_structure.py` enforces), ground their checklists in **fetched** - authoritative sources (OWASP/CIS/vendor docs) cited in the skill body, and - land with a golden-diff eval fixture under `evals/fixtures/` (ideally also a - `-clean` one). + `tests/test_skill_structure.py` enforces), ground their checklists in + **fetched** authoritative sources (OWASP/CIS/vendor docs) cited in the + skill body, and land with a golden-diff eval fixture under `evals/fixtures/` + (ideally also a `-clean` one). - Review skills report in the shared shape of `docs/finding-output.md` and inline its one-paragraph severity rubric word for word in `## Output` - (`tests/test_skill_structure.py` compares them); change the rubric in the doc, - `skilldeck.lint.SEVERITY_RUBRIC` and every skill together. Respect its "Which skill owns what" table: a - defect is reported once, by its owning skill. + (`tests/test_skill_structure.py` compares them); change the rubric in the + doc, `skilldeck.lint.SEVERITY_RUBRIC` and every skill together. Respect its + "Which skill owns what" table: a defect is reported once, by its owning + skill. ## Shipping - PRs squash-merge to main: `gh pr merge --squash --delete-branch` after CI diff --git a/docs/authoring-skills.md b/docs/authoring-skills.md index 1d73213..c96745e 100644 --- a/docs/authoring-skills.md +++ b/docs/authoring-skills.md @@ -25,15 +25,21 @@ uv run --extra dev skilldeck new my-review --category security \ --description "Review pending changes for ..." # write the content: replace every TODO(author) placeholder in # src/skilldeck/skills/my-review/skill.md, plant a defect in -# evals/fixtures/my-review/, list the skill in docs/finding-output.md +# evals/fixtures/my-review/ and add its SAMPLE_REPORTS entry in +# tests/test_eval_fixtures.py, list the skill in docs/finding-output.md uv run --extra dev skilldeck validate my-review uv run --extra dev python scripts/build_plugin.py # regenerate the plugin tree -uv run --extra dev skilldeck validate # every skill, before you push +uv run --extra dev skilldeck validate # every skill +uv run --extra dev pytest # what validate can't check ``` +`validate` covers the rules on this page; the test suite also checks what it +cannot, such as each planted fixture's `SAMPLE_REPORTS`. Before you push, run +the full check suite in [CONTRIBUTING.md](../CONTRIBUTING.md). + Run the commands from the checkout with `uv run --extra dev`, so they use the -checkout's own code rather than an installed skilldeck (`validate` notes it -when they differ). +checkout's own code. `validate` runs the eval-fixture and generated-output +checks only then (see [Trust](#trust)). ## Organization skills @@ -106,17 +112,33 @@ skills directory is checked. For each skill it checks: - the `skill.md` structure and cited sources, the rules in `skilldeck.lint` that the test suite also applies to every bundled skill; - that no `TODO(author)` placeholder is left, and that the directory holds - nothing but the two skill files; + nothing but the two skill files, neither of them a symlink; - that every adapter for the skill's agents, legacy formats included, can render it, and that its [catalog](catalog.md) entry builds. A skill in a checkout's `src/skilldeck/skills` also gets the repository checks: its eval fixtures load, have `base/` and `change/` with every planted -file in `change/`, and include a planted one (through `evals/run_evals.py`, -without running any agent); it is listed in +file in `change/`, use keywords that describe each defect rather than echo +its code, keep a clean-diff fixture's tolerance small, and include a planted +one (through `evals/run_evals.py`, without running any agent); it is listed in `docs/finding-output.md`; and the generated plugin tree and content manifests -are current (`scripts/build_plugin.py --check`, run in process). Every check -reads local files only; nothing touches the network or calls an agent. +are current (`scripts/build_plugin.py --check`, run in process). Nothing +touches the network or calls an agent. + +### Trust + +`validate` never runs code from the tree it checks. The eval-fixture and +generated-output checks import that checkout's own `evals/run_evals.py` and +`scripts/build_plugin.py`, so they run only when the skilldeck doing the +validating *is* that checkout's code: its `src/skilldeck` is the running +package, as with `uv run --extra dev skilldeck validate` inside it. For any +other tree that looks like a checkout (a fork, a downloaded archive, a +checkout validated by an installed skilldeck), they are skipped and the +report says to run that command inside it; the text-only +`docs/finding-output.md` check still applies. Nothing in a skill directory is +followed through a symlink: a symlinked `meta.yaml` or `skill.md` is reported +(`skill.symlink`) and not read, so its target's path and contents never reach +the report. Each problem names the file (and line, where there is one), the rule and how to fix it: @@ -135,7 +157,6 @@ keys, problems sorted by skill, file, line and rule, one final newline): ```json { - "notes": [], "ok": false, "problems": [ { @@ -162,10 +183,8 @@ keys, problems sorted by skill, file, line and rule, one final newline): ``` A problem that belongs to the whole checkout (a stale generated file) has -`skill: null`. `skipped` lists checks that could not apply, and why; -`notes` gives advice that is not a problem, such as a `validate` that runs -other code than the checkout's. `schema_version` changes only if the shape -changes incompatibly. +`skill: null`. `skipped` lists checks that could not apply, and why. +`schema_version` changes only if the shape changes incompatibly. ### Rules @@ -189,6 +208,7 @@ changes incompatibly. | `body.missing` | error | The skill directory has a `skill.md`. | | `body.encoding` | error | `skill.md` is UTF-8. | | `skill.unexpected-file` | error | The skill directory holds only `meta.yaml` and `skill.md`. | +| `skill.symlink` | error | Nothing in the skill directory is a symlink. | | `structure.heading` | error | `skill.md` opens with a `# Title` whose words spell the skill name. | | `structure.section` | error | `skill.md` has the `## Scope` and `## Output` sections. | | `structure.phrase` | error | `skill.md` carries the instructions every review skill shares. | @@ -204,6 +224,8 @@ changes incompatibly. | `render.failed` | error | Every adapter for the skill's agents can render it. | | `catalog.entry` | error | The skill's catalog entry builds. | | `eval.fixture-missing` | incomplete | Checkout only: an eval fixture with a planted defect exercises the skill. | +| `eval.keyword-echo` | error | Checkout only: a plant's keywords describe the defect instead of echoing the planted code. | +| `eval.clean-tolerance` | error | Checkout only: a clean-diff fixture tolerates at most 2 findings. | | `eval.fixture-invalid` | error | Checkout only: the skill's eval fixtures load, and their planted files are in `change/`. | | `docs.finding-output` | incomplete | Checkout only: `docs/finding-output.md` lists the skill and its classifier. | | `generated.stale` | error | Checkout only: the generated plugin tree and content manifests match the skills. | diff --git a/evals/run_evals.py b/evals/run_evals.py index 47a92ca..58de83e 100644 --- a/evals/run_evals.py +++ b/evals/run_evals.py @@ -97,6 +97,8 @@ ) #: the default cap on planned runs (fixtures x repeats) per invocation DEFAULT_MAX_RUNS = 50 +#: the most findings a clean-diff fixture (no plants) may tolerate +MAX_CLEAN_FINDINGS = 2 DEFAULT_TIMEOUT = 600 #: seconds allowed for a harness's ``--version`` probe VERSION_PROBE_TIMEOUT = 30 @@ -1052,6 +1054,44 @@ def fixture_layout_problems(fixture: Fixture) -> list[str]: return problems +def echoed_keywords(fixture: Fixture) -> list[str]: + """One message per plant whose keywords appear verbatim in its planted file. + + A keyword copied from the planted code (a variable, an event name, a + CIDR) is satisfied by any report that quotes the line, whether or not it + identified the defect. Same whole-word matching as the scorer. A plant + file that is missing or unreadable is :func:`fixture_layout_problems`' + to report. + """ + problems = [] + for plant in fixture.plants: + try: + code = (fixture.path / "change" / plant.file).read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + echoed = [k for k in plant.keywords if mentions(code, k)] + if echoed: + problems.append( + f"{plant.file}: keyword(s) {echoed} appear verbatim in the planted " + "code; keywords must describe the defect, not echo the code" + ) + return problems + + +def clean_tolerance_problem(fixture: Fixture) -> str | None: + """Why a clean-diff fixture's ``max-findings`` is too lenient, if it is. + + With no plants, ``max-findings`` is the fixture's false-positive + tolerance, so it stays small. + """ + if fixture.plants or fixture.max_findings <= MAX_CLEAN_FINDINGS: + return None + return ( + f"a clean-diff fixture tolerates at most {MAX_CLEAN_FINDINGS} findings, " + f"not {fixture.max_findings}" + ) + + def rendered_digest(adapter: str, skill: Skill) -> str: """The hash an install stamp records: the rendered skill, stamp excluded. diff --git a/src/skilldeck/authoring.py b/src/skilldeck/authoring.py index be6e927..770048d 100644 --- a/src/skilldeck/authoring.py +++ b/src/skilldeck/authoring.py @@ -10,16 +10,19 @@ an organization can author its own skills with the same checks; nothing here writes into the installed package. -Every check is local: files are read, adapters render in memory, and the -checkout's own ``evals/run_evals.py`` and ``scripts/build_plugin.py`` are -imported to validate fixtures and generated output. No network, no agent. +Every check is local: files are read, adapters render in memory, and no +network or agent is involved. The eval-fixture and generated-output checks +import the checkout's own ``evals/run_evals.py`` and +``scripts/build_plugin.py``, so they run only for the checkout this skilldeck +itself runs from (:func:`_trusted_checkout`); validate never executes code +from any other tree. Symlinks inside a skill are reported, never followed. """ from __future__ import annotations -import contextlib import hashlib import importlib.util +import os import re import sys from collections.abc import Iterable, Sequence @@ -37,7 +40,9 @@ INCOMPLETE, PLACEHOLDER, SEVERITY_RUBRIC, + SKILL_FILES, Problem, + bundle_problems, description_problems, finding_output_problems, placeholder_problems, @@ -50,6 +55,7 @@ NAME_RE, Skill, SkillError, + check_meta, discover_skills, load_skill, replacement_errors, @@ -60,8 +66,6 @@ FIXTURES_SUBDIR = Path("evals", "fixtures") FINDING_OUTPUT_DOC = Path("docs", "finding-output.md") _PROJECT_NAME_RE = re.compile(r'^name\s*=\s*"skilldeck"\s*$', re.M) -#: the only files a skill directory holds -SKILL_FILES = ("meta.yaml", "skill.md") #: a new skill's first version INITIAL_VERSION = "0.1.0" #: bumped on a breaking change to ``skilldeck validate --json`` @@ -135,9 +139,13 @@ def installed_package_dir() -> Path | None: def display_path(path: Path, base: Path | None = None) -> str: """``path`` as a POSIX path, relative to ``base`` (the working directory) - when it is under it.""" - base = (base or Path.cwd()).resolve() - absolute = (base / path).resolve() + when it is under it. + + Symlinks are not resolved, so a path shows the way it was given rather + than where a link points. + """ + base = Path(os.path.abspath(base or Path.cwd())) + absolute = Path(os.path.abspath(base / path)) try: return absolute.relative_to(base).as_posix() except ValueError: @@ -366,7 +374,7 @@ class SkillStatus: name: str #: the skill directory, as :func:`display_path` shows it path: str - #: whether the repository checks applied (a checkout's skills directory) + #: whether it is in a checkout's skills directory (repository checks) checkout: bool #: ok, incomplete (authoring work remains, nothing wrong) or invalid status: str @@ -385,7 +393,6 @@ class Report: skills: list[SkillStatus] = field(default_factory=list) problems: list[Problem] = field(default_factory=list) skipped: list[Skipped] = field(default_factory=list) - notes: list[str] = field(default_factory=list) @property def ok(self) -> bool: @@ -420,20 +427,33 @@ def to_json(self) -> dict[str, object]: {"check": skipped.check, "reason": skipped.reason} for skipped in self.skipped ], - "notes": list(self.notes), } +def _trusted_checkout(checkout: Checkout) -> bool: + """Whether ``checkout`` is the source of the skilldeck running now. + + The eval-fixture and generated-output checks import the checkout's own + ``evals/run_evals.py`` and ``scripts/build_plugin.py``. That is only + safe for the code already running: any tree can look like a skilldeck + checkout (a fork, a downloaded archive), and validating it must never run + what it contains. + """ + running = Path(__file__).resolve().parent + return (checkout.root / "src" / "skilldeck").resolve() == running + + _SCRIPTS: dict[Path, ModuleType] = {} def _load_script(path: Path) -> ModuleType: - """Import a checkout script (``evals/run_evals.py``, + """Import a trusted checkout's script (``evals/run_evals.py``, ``scripts/build_plugin.py``) by path, once per process. - The scripts put the checkout's directories on ``sys.path`` to import - their helpers; that is undone afterwards, since skilldeck itself is - already imported. + Call it only for a checkout :func:`_trusted_checkout` accepts. The + scripts put the checkout's directories on ``sys.path`` to import their + helpers; that is undone afterwards, since skilldeck itself is already + imported. """ path = path.resolve() if path in _SCRIPTS: @@ -473,6 +493,14 @@ def _status(problems: Iterable[Problem]) -> str: return "ok" +def _read_text(path: Path) -> str | None: + """``path``'s text, or None if it is unreadable or not UTF-8.""" + try: + return path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None + + class _Checker: """Validates the skills of one run, loading each directory's siblings and each checkout's scripts only once.""" @@ -489,6 +517,8 @@ def siblings(self, skills_dir: Path) -> list[Skill]: if skills_dir not in self._siblings: loaded = [] for child in skill_dirs(skills_dir): + if any((child / name).is_symlink() for name in SKILL_FILES): + continue try: loaded.append(load_skill(child, set(ADAPTERS))) except SkillError: @@ -497,7 +527,7 @@ def siblings(self, skills_dir: Path) -> list[Skill]: return self._siblings[skills_dir] def skill( - self, skill_dir: Path, checkout: Checkout | None + self, skill_dir: Path, checkout: Checkout | None, trusted: bool ) -> tuple[list[Problem], list[Skipped]]: name = skill_dir.name meta_path = skill_dir / "meta.yaml" @@ -505,65 +535,65 @@ def skill( problems: list[Problem] = [] skipped: list[Skipped] = [] + # a symlinked skill file is reported and never read (see + # lint.bundle_problems); the other file is still checked + linked = {n for n in SKILL_FILES if (skill_dir / n).is_symlink()} + if skill_dir.is_dir(): + problems += bundle_problems(skill_dir, self.show) + skill: Skill | None = None try: - skill = load_skill(skill_dir, set(ADAPTERS)) + if not linked: + skill = load_skill(skill_dir, set(ADAPTERS)) + elif "meta.yaml" not in linked: + check_meta(skill_dir, set(ADAPTERS)) except SkillError as exc: problems.append( Problem( self.show(exc.file or skill_dir), exc.rule or "meta.syntax", exc.detail, - skill=name, + exc.line, + name, ) ) reported = {problem.rule for problem in problems} body = skill.body if skill is not None else None - if body is None and body_path.is_file(): - try: - body = body_path.read_text(encoding="utf-8") - except UnicodeDecodeError as exc: - if "body.encoding" not in reported: + if body is None and "skill.md" not in linked: + if not body_path.is_file(): + if "body.missing" not in reported: + problems.append( + Problem( + self.show(body_path), + "body.missing", + "missing skill.md", + None, + name, + ) + ) + else: + body = _read_text(body_path) + if body is None and not reported & {"body.encoding", "body.missing"}: problems.append( Problem( self.show(body_path), "body.encoding", - f"skill.md is not valid UTF-8: {exc}", - skill=name, + "skill.md is not valid UTF-8", + None, + name, ) ) - except OSError: - pass - elif body is None and "body.missing" not in reported: - problems.append( - Problem( - self.show(body_path), "body.missing", "missing skill.md", skill=name - ) - ) if body is not None: where = self.show(body_path) problems += structure_problems(name, body, where) problems += reference_problems(name, body, where) problems += placeholder_problems(body, where, name) - # an unreadable meta.yaml is the registry's to report - with contextlib.suppress(OSError, UnicodeDecodeError): - problems += placeholder_problems( - meta_path.read_text(encoding="utf-8"), self.show(meta_path), name - ) - - if skill_dir.is_dir(): - problems += [ - Problem( - self.show(child), - "skill.unexpected-file", - f"{child.name} is not part of a skill (only " - f"{' and '.join(SKILL_FILES)} are)", - skill=name, - ) - for child in sorted(skill_dir.iterdir()) - if child.name not in SKILL_FILES - ] + if "meta.yaml" not in linked: + # an unreadable meta.yaml is the registry's to report + meta_text = _read_text(meta_path) + if meta_text is not None: + problems += placeholder_problems(meta_text, self.show(meta_path), name) if skill is None: skipped.append( @@ -590,18 +620,18 @@ def skill( skill, ] ) - if exc.file == skill_dir / "meta.yaml" + if exc.file == meta_path ] if checkout is not None: doc = checkout.root / FINDING_OUTPUT_DOC - if doc.is_file(): - problems += finding_output_problems( - doc.read_text(encoding="utf-8"), name, self.show(doc) - ) - fixture_problems, fixture_skipped = self._fixtures(name, checkout) - problems += fixture_problems - skipped += fixture_skipped + text = _read_text(doc) if doc.is_file() and not doc.is_symlink() else None + if text is not None: + problems += finding_output_problems(text, name, self.show(doc)) + if trusted: + fixture_problems, fixture_skipped = self._fixtures(name, checkout) + problems += fixture_problems + skipped += fixture_skipped return problems, skipped def _rendering(self, skill: Skill, meta_path: Path) -> list[Problem]: @@ -641,7 +671,11 @@ def _fixtures( self, name: str, checkout: Checkout ) -> tuple[list[Problem], list[Skipped]]: """The eval fixtures that exercise skill ``name``: present, loadable, - buildable, free of placeholders, and at least one planted.""" + well formed, free of placeholders, and at least one planted. + + Uses the checkout's own fixture loader, so the checkout must be + trusted (see :func:`_trusted_checkout`). + """ fixtures_dir = checkout.fixtures_dir names = {child.name for child in skill_dirs(checkout.skills_dir)} | {name} candidates = ( @@ -711,14 +745,31 @@ def _fixtures( skill=name, ) ) + problems += [ + Problem( + self.show(expected), "eval.keyword-echo", str(message), None, name + ) + for message in runner.echoed_keywords(fixture) + ] + tolerance = runner.clean_tolerance_problem(fixture) + if tolerance: + problems.append( + Problem( + self.show(expected), + "eval.clean-tolerance", + tolerance, + None, + name, + ) + ) for file in sorted(path.rglob("*")): - if not file.is_file() or "__pycache__" in file.parts: + if file.is_symlink() or not file.is_file(): continue - try: - text = file.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): + if "__pycache__" in file.parts: continue - problems += placeholder_problems(text, self.show(file), name) + text = _read_text(file) + if text is not None: + problems += placeholder_problems(text, self.show(file), name) if broken: return problems, skipped where = self.show(fixtures_dir / name) @@ -753,7 +804,8 @@ def _fixtures( return problems, skipped def generated(self, checkout: Checkout) -> tuple[list[Problem], list[Skipped]]: - """``scripts/build_plugin.py --check``, in process.""" + """``scripts/build_plugin.py --check``, in process; the checkout must + be trusted (see :func:`_trusted_checkout`).""" try: discover_skills(checkout.skills_dir, known_agents=set(ADAPTERS)) except SkillError: @@ -810,26 +862,35 @@ def validate(skill_paths: Sequence[Path], base: Path | None = None) -> Report: A skill in a checkout's canonical skills directory also gets the repository checks, and each such checkout's generated output is checked - once. Problems are sorted: by skill, then file, line and rule, with - checkout-wide ones last. ``base`` (default: the working directory) is - what paths are shown relative to. + once. The checks that run a checkout's own scripts (eval fixtures, + generated output) apply only to the checkout this skilldeck runs from; + for any other they are skipped, never run. Problems are sorted: by + skill, then file, line and rule, with checkout-wide ones last. ``base`` + (default: the working directory) is what paths are shown relative to. """ - checker = _Checker((base or Path.cwd()).resolve()) + checker = _Checker(Path(os.path.abspath(base or Path.cwd()))) report = Report() checkouts: dict[Path, Checkout] = {} - outside: set[Path] = set() + outside: set[str] = set() seen: set[Path] = set() + trusted: dict[Path, bool] = {} for path in skill_paths: - skill_dir = path.resolve() - if skill_dir in seen: + # shown as given (a symlinked skills directory keeps its name); + # compared resolved + skill_dir = Path(os.path.abspath(path)) + if skill_dir.resolve() in seen: continue - seen.add(skill_dir) + seen.add(skill_dir.resolve()) checkout = checkout_of(skill_dir.parent) if checkout is None: - outside.add(skill_dir.parent) + outside.add(checker.show(skill_dir.parent)) else: checkouts[checkout.root] = checkout - problems, skipped = checker.skill(skill_dir, checkout) + if checkout.root not in trusted: + trusted[checkout.root] = _trusted_checkout(checkout) + problems, skipped = checker.skill( + skill_dir, checkout, checkout is not None and trusted[checkout.root] + ) report.problems += problems report.skipped += skipped report.skills.append( @@ -844,22 +905,27 @@ def validate(skill_paths: Sequence[Path], base: Path | None = None) -> Report: report.skipped.append( Skipped( "repository checks (eval fixtures, finding-output doc, generated " - f"output) for {checker.show(skills_dir)}", + f"output) for {skills_dir}", "not the src/skilldeck/skills directory of a skilldeck checkout", ) ) running = Path(__file__).resolve().parent for root, checkout in sorted(checkouts.items()): + if not trusted[root]: + report.skipped.append( + Skipped( + "eval fixture and generated-output checks for " + f"{checker.show(root)}", + "they run that checkout's own scripts, and this skilldeck " + f"runs from {checker.show(running)}, not from its " + "src/skilldeck; run `uv run --extra dev skilldeck validate` " + "inside that checkout", + ) + ) + continue problems, skipped = checker.generated(checkout) report.problems += problems report.skipped += skipped - if running != (root / "src" / "skilldeck").resolve(): - report.notes.append( - f"this skilldeck runs from {checker.show(running)}, not from " - f"{checker.show(root / 'src' / 'skilldeck')}; run `uv run " - "--extra dev skilldeck validate` in the checkout so the checks " - "use its code" - ) report.skills.sort(key=lambda skill: (skill.name, skill.path)) report.problems.sort(key=Problem.sort_key) report.skipped.sort(key=lambda skipped: (skipped.check, skipped.reason)) @@ -882,14 +948,12 @@ def format_report(report: Report) -> list[str]: if skill.status == "ok": detail = "" elif skill.status == "incomplete": - detail = f" (no errors; {incomplete} authoring item(s) remain)" + detail = f" (no errors in the skill; {incomplete} authoring item(s) remain)" else: detail = f" ({errors} error(s), {incomplete} incomplete)" lines.append(f"{skill.name}: {skill.status}{detail}") for skipped in report.skipped: lines.append(f"skipped {skipped.check}: {skipped.reason}") - for note in report.notes: - lines.append(f"note: {note}") counts = { status: sum(skill.status == status for skill in report.skills) for status in ("ok", "incomplete", "invalid") diff --git a/src/skilldeck/cli.py b/src/skilldeck/cli.py index 9cad73a..6be954e 100644 --- a/src/skilldeck/cli.py +++ b/src/skilldeck/cli.py @@ -2,6 +2,7 @@ from __future__ import annotations +import shlex import stat from collections.abc import Collection, Iterable from itertools import groupby @@ -880,7 +881,7 @@ def new( else: check = ( "skilldeck validate --skills-dir " - f"{authoring.display_path(skills_dir)} {name}" + f"{shlex.quote(authoring.display_path(skills_dir))} {name}" ) steps = [ f"Replace every {PLACEHOLDER} placeholder. Ground the checklist in " @@ -890,11 +891,19 @@ def new( if in_checkout: steps += [ f"Build the eval fixture in evals/fixtures/{name}/ (see " - "evals/README.md) and add the skill to docs/finding-output.md.", + "evals/README.md), add its SAMPLE_REPORTS entry in " + "tests/test_eval_fixtures.py, and add the skill to " + "docs/finding-output.md.", "Regenerate the plugin tree: uv run --extra dev python " "scripts/build_plugin.py", ] steps.append(f"Check it: {check}") + if in_checkout: + steps.append( + "Before you push, run the full check suite in CONTRIBUTING.md " + "(uv run --extra dev pytest checks what validate cannot, such as " + "SAMPLE_REPORTS)." + ) click.echo("\nNext:") for number, step in enumerate(steps, start=1): click.echo(f" {number}. {step}") @@ -946,6 +955,8 @@ def validate(targets: tuple[str, ...], skills_dir: Path | None, as_json: bool) - ) paths: list[Path] = [] for target in targets: + if not target.strip(): + raise click.UsageError("an empty skill name or path") if _is_path_argument(target): path = Path(target) if not path.is_dir(): diff --git a/src/skilldeck/lint.py b/src/skilldeck/lint.py index 8f0c616..b4581a0 100644 --- a/src/skilldeck/lint.py +++ b/src/skilldeck/lint.py @@ -22,7 +22,9 @@ from __future__ import annotations import re +from collections.abc import Callable from dataclasses import dataclass +from pathlib import Path #: marks content a skill author still has to write; ``skilldeck new`` puts it #: wherever the template cannot know the domain, and validate reports it @@ -31,6 +33,9 @@ ERROR = "error" INCOMPLETE = "incomplete" +#: the only files a skill directory holds +SKILL_FILES = ("meta.yaml", "skill.md") + @dataclass(frozen=True) class Rule: @@ -138,6 +143,12 @@ class Rule: "move the file out of the skill directory: installs carry only " "meta.yaml and skill.md, and provenance --verify rejects other files", ), + "skill.symlink": Rule( + ERROR, + "nothing in the skill directory is a symlink", + "replace the symlink with a regular file; validate never follows one, " + "so a symlinked meta.yaml or skill.md is not checked at all", + ), "structure.heading": Rule( ERROR, "skill.md opens with a '# Title' whose words spell the skill name", @@ -223,6 +234,18 @@ class Rule: "add an eval fixture that plants a defect the skill must find; see " "evals/README.md#adding-a-fixture", ), + "eval.keyword-echo": Rule( + ERROR, + "a plant's keywords describe the defect instead of echoing the code", + "use words a correct finding would use to describe the defect, not " + "identifiers or values copied from the planted file; see " + "evals/README.md#adding-a-fixture", + ), + "eval.clean-tolerance": Rule( + ERROR, + "a clean-diff fixture tolerates at most 2 findings", + "lower max-findings to 0, or 1 or 2 for hygiene nits", + ), "eval.fixture-invalid": Rule( ERROR, "the skill's eval fixtures load, and their planted files are in change/", @@ -284,6 +307,42 @@ def sort_key(self) -> tuple[bool, str, str, int, str, str]: ) +# -- the skill directory ---------------------------------------------------------- + + +def bundle_problems( + skill_dir: Path, show: Callable[[Path], str] = Path.as_posix +) -> list[Problem]: + """What in ``skill_dir`` is not one of :data:`SKILL_FILES`, or is a symlink. + + Nothing is read, and a symlink is never followed: it could point at any + file, whose path or contents must not end up in a report. ``show`` turns + a path into the one reported. + """ + problems = [] + for child in sorted(skill_dir.iterdir()): + if child.is_symlink(): + problems.append( + Problem( + show(child), + "skill.symlink", + f"{child.name} is a symlink, which validate does not follow", + skill=skill_dir.name, + ) + ) + elif child.name not in SKILL_FILES: + problems.append( + Problem( + show(child), + "skill.unexpected-file", + f"{child.name} is not part of a skill (only " + f"{' and '.join(SKILL_FILES)} are)", + skill=skill_dir.name, + ) + ) + return problems + + # -- the structural template -------------------------------------------------- #: the one-paragraph severity rubric every review skill's ``## Output`` @@ -381,7 +440,12 @@ def add(rule: str, message: str, line: int | None = None) -> None: f"heading {first!r} does not spell the skill name {name!r}", 1, ) - headings = {heading: _heading(body, heading) for heading in REQUIRED_HEADINGS} + # Scope and Output are looked up whatever REQUIRED_HEADINGS says: their + # patterns below depend on them + headings = { + heading: _heading(body, heading) + for heading in (*REQUIRED_HEADINGS, "Scope", "Output") + } for heading, why in REQUIRED_HEADINGS.items(): if headings[heading] is None: add("structure.section", f"missing '## {heading}' ({why})") diff --git a/src/skilldeck/registry.py b/src/skilldeck/registry.py index be6cc08..034b363 100644 --- a/src/skilldeck/registry.py +++ b/src/skilldeck/registry.py @@ -52,8 +52,9 @@ class SkillError(Exception): For a malformed skill, ``rule`` names the metadata rule it breaks (e.g. ``meta.version``; ``skilldeck validate`` lists them), ``file`` is the file - that breaks it, and ``detail`` is the message without the leading skill - directory. Other errors leave ``rule`` and ``file`` None. + that breaks it, ``line`` the 1-based line when known, and ``detail`` a + one-line message without the leading skill directory. Other errors leave + ``rule``, ``file`` and ``line`` None. """ def __init__( @@ -63,11 +64,13 @@ def __init__( rule: str | None = None, file: Path | None = None, detail: str | None = None, + line: int | None = None, ) -> None: super().__init__(message) self.rule = rule self.file = file self.detail = message if detail is None else detail + self.line = line def _invalid( @@ -115,6 +118,54 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> if not body_path.is_file(): raise _invalid(skill_dir, "body.missing", "missing skill.md", "skill.md") + meta = _load_meta(skill_dir, known_agents) + + try: + body = body_path.read_text(encoding="utf-8") + except UnicodeDecodeError as exc: + raise _invalid( + skill_dir, + "body.encoding", + f"skill.md is not valid UTF-8: {exc}", + "skill.md", + ) from exc + + return Skill( + name=meta.name, + description=meta.description, + category=meta.category, + version=meta.version, + supported_agents=meta.supported_agents, + body=body, + path=skill_dir, + deprecated=meta.deprecated, + ) + + +def check_meta(skill_dir: Path, known_agents: Collection[str] | None = None) -> None: + """Validate ``skill_dir/meta.yaml`` alone, as :func:`load_skill` does. + + For ``skilldeck validate``, when ``skill.md`` cannot be read. Raises + :class:`SkillError` as :func:`load_skill` would. + """ + if not (skill_dir / "meta.yaml").is_file(): + raise _invalid(skill_dir, "meta.missing", "missing meta.yaml") + _load_meta(skill_dir, known_agents) + + +@dataclass(frozen=True) +class _Meta: + name: str + description: str + category: str + version: str + supported_agents: tuple[str, ...] + deprecated: Deprecation | None + + +def _load_meta(skill_dir: Path, known_agents: Collection[str] | None) -> _Meta: + """Read and validate ``skill_dir/meta.yaml``, which exists.""" + meta_path = skill_dir / "meta.yaml" try: meta = yaml.safe_load(meta_path.read_text(encoding="utf-8")) or {} except UnicodeDecodeError as exc: @@ -122,8 +173,12 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> skill_dir, "meta.encoding", f"meta.yaml is not valid UTF-8: {exc}" ) from exc except yaml.YAMLError as exc: - raise _invalid( - skill_dir, "meta.syntax", f"meta.yaml is not valid YAML: {exc}" + raise SkillError( + f"{skill_dir}: meta.yaml is not valid YAML: {exc}", + rule="meta.syntax", + file=meta_path, + detail=f"meta.yaml is not valid YAML: {_yaml_problem(exc)}", + line=_yaml_line(exc), ) from exc if not isinstance(meta, dict): raise _invalid(skill_dir, "meta.syntax", "meta.yaml must be a YAML mapping") @@ -161,7 +216,7 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> description = _require_str(skill_dir, meta, "description") # Any line boundary ``str.splitlines`` knows, not just \n and \r: YAML - # double-quoted escapes such as "
" or "\x85" also break the one-line + # double-quoted escapes such as "\u2028" or "\x85" also break the one-line # ``skilldeck list`` output. if description.splitlines() != [description]: raise _invalid( @@ -218,28 +273,32 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> else None ) - try: - body = body_path.read_text(encoding="utf-8") - except UnicodeDecodeError as exc: - raise _invalid( - skill_dir, - "body.encoding", - f"skill.md is not valid UTF-8: {exc}", - "skill.md", - ) from exc - - return Skill( + return _Meta( name=name, description=description, category=category, version=raw_version, supported_agents=tuple(agents), - body=body, - path=skill_dir, deprecated=deprecated, ) +def _yaml_problem(exc: yaml.YAMLError) -> str: + """What is wrong with the YAML, on one line and without the source + excerpt PyYAML quotes (which can echo a file's contents).""" + if isinstance(exc, yaml.MarkedYAMLError) and exc.problem: + if exc.context: + return f"{exc.problem} ({exc.context})" + return exc.problem + return " ".join(str(exc).split()) + + +def _yaml_line(exc: yaml.YAMLError) -> int | None: + """The 1-based line PyYAML found the problem on, if it knows.""" + mark = getattr(exc, "problem_mark", None) + return mark.line + 1 if mark is not None else None + + def _require_str(skill_dir: Path, meta: dict[Any, Any], field: str) -> str: """Return ``meta[field]`` if it is a non-blank string, else fail loudly.""" value = meta[field] diff --git a/tests/test_authoring.py b/tests/test_authoring.py index 942c1f9..9237c89 100644 --- a/tests/test_authoring.py +++ b/tests/test_authoring.py @@ -112,6 +112,11 @@ def test_new_scaffolds_meta_and_skeleton(skills_dir): assert b"\r" not in path.read_bytes() +def test_new_quotes_the_directory_in_its_hint(skills_dir): + out = _new(skills_dir.parent / "my skills").stdout + assert f"skilldeck validate --skills-dir 'my skills' {NAME}" in out + + def test_new_takes_description_and_agents(skills_dir): _new( skills_dir, @@ -264,9 +269,23 @@ def test_malformed_metadata(completed): def test_unparseable_metadata(completed): - (completed / "meta.yaml").write_text("name: [unclosed\n", encoding="utf-8") + (completed / "meta.yaml").write_text( + "name: x\ndescription: [unclosed\ncategory: review\n", encoding="utf-8" + ) problem = _problem(_report("--skills-dir", completed.parent), "meta.syntax") assert problem["path"] == f"skills/{NAME}/meta.yaml" + assert problem["line"] == 3 + # one line, without PyYAML's excerpt of the file + assert problem["message"] == ( + "meta.yaml is not valid YAML: expected ',' or ']', but got ':' " + "(while parsing a flow sequence)" + ) + out = _invoke("validate", "--skills-dir", completed.parent, code=1).stdout + assert f"skills/{NAME}/meta.yaml:3: error [meta.syntax]" in out + assert "category: review" not in out + # the registry's own message is unchanged + with pytest.raises(registry.SkillError, match="(?s)in .*line 2, column"): + registry.load_skill(completed) def test_unsupported_agent(completed): @@ -440,6 +459,12 @@ def test_validate_needs_a_skills_directory_outside_a_checkout(skills_dir): assert "not inside a skilldeck checkout" in result.stderr +@pytest.mark.parametrize("target", ["", " "]) +def test_validate_rejects_an_empty_target(completed, target): + result = _invoke("validate", "--skills-dir", completed.parent, target, code=2) + assert "empty skill name or path" in result.stderr + + def test_validate_rejects_an_unknown_skill(completed): result = _invoke("validate", "--skills-dir", completed.parent, "nope", code=2) assert "no skill 'nope' in skills" in result.stderr @@ -508,6 +533,9 @@ def checkout(tmp_path, monkeypatch): # the copied build_plugin.py imports its own _pyproject; keep that copy # out of the other tests' way monkeypatch.delitem(sys.modules, "_pyproject", raising=False) + # its scripts are this repository's own, so running them is safe; a + # real validate runs only the scripts of the checkout it runs from + monkeypatch.setattr(authoring, "_trusted_checkout", lambda checkout: True) return root @@ -550,6 +578,7 @@ def test_checkout_lifecycle(checkout, capsys): fixture_dir = checkout / "evals" / "fixtures" / NAME assert f"created evals/fixtures/{NAME}/expected.yaml" in out assert "scripts/build_plugin.py" in out + assert "SAMPLE_REPORTS" in out and "uv run --extra dev pytest" in out # the fixture skeleton is a valid (clean-diff) fixture, so committing it # breaks nothing, but validate still reports what is left to write @@ -615,6 +644,24 @@ def test_checkout_missing_eval_fixture(checkout): assert f"add evals/fixtures/{NAME}/" in problem["remediation"] +def test_checkout_keywords_must_not_echo_the_code(checkout): + _invoke("new", NAME, "--category", "review") + fixture_dir = checkout / "evals" / "fixtures" / NAME + _plant(fixture_dir) + _edit(fixture_dir / "expected.yaml", "[overspeed]", "[overspeed, spin]") + problem = _problem(_report(NAME), "eval.keyword-echo") + assert problem["path"] == f"evals/fixtures/{NAME}/expected.yaml" + assert "['spin']" in problem["message"] + + +def test_checkout_clean_fixture_tolerance(checkout): + _invoke("new", NAME, "--category", "review") + fixture_dir = checkout / "evals" / "fixtures" / NAME + _edit(fixture_dir / "expected.yaml", "max-findings: 0", "max-findings: 5") + problem = _problem(_report(NAME), "eval.clean-tolerance") + assert "at most 2" in problem["message"] + + def test_checkout_invalid_eval_fixture(checkout): _invoke("new", NAME, "--category", "review") fixture_dir = checkout / "evals" / "fixtures" / NAME @@ -646,3 +693,112 @@ def test_bundled_skills_validate_clean(): assert report.skipped == [] assert {skill.status for skill in report.skills} == {"ok"} assert all(skill.checkout for skill in report.skills) + + +# --- symlinks: reported, never followed ------------------------------------------- + + +@pytest.mark.parametrize("linked", ["meta.yaml", "skill.md"]) +def test_symlinked_skill_file_is_reported_and_not_read( + completed, tmp_path, symlink, linked +): + secret = tmp_path / "outside" / "secret.env" + secret.parent.mkdir() + secret.write_text("API_TOKEN: [hunter2\n", encoding="utf-8") + (completed / linked).unlink() + symlink(completed / linked, secret) + out = _invoke("validate", "--json", "--skills-dir", completed.parent, code=1) + assert "hunter2" not in out.stdout + assert "outside" not in out.stdout + report = json.loads(out.stdout) + problem = _problem(report, "skill.symlink") + assert problem["path"] == f"skills/{NAME}/{linked}" + assert report["skills"][0]["status"] == "invalid" + # the other file is still checked + other = "skill.md" if linked == "meta.yaml" else "meta.yaml" + _edit(completed / other, "0.1.0" if other == "meta.yaml" else SOURCE, "x") + report = _report("--skills-dir", completed.parent) + assert {p["path"] for p in report["problems"]} >= {f"skills/{NAME}/{other}"} + + +def test_symlinked_skills_directory_shows_the_path_given(completed, tmp_path, symlink): + link = tmp_path / "linked-skills" + symlink(link, completed.parent) + _edit(completed / "skill.md", "\n## Scope\n", "\n## Where to look\n") + report = _report("--skills-dir", "linked-skills") + assert report["skills"][0]["path"] == f"linked-skills/{NAME}" + problem = _problem(report, "structure.section") + assert problem["path"] == f"linked-skills/{NAME}/skill.md" + + +# --- validate never runs a tree's own code ---------------------------------------- + +SENTINEL_SCRIPT = textwrap.dedent( + """\ + from pathlib import Path + + Path(__file__).resolve().parent.parent.joinpath("RAN").write_text( + __file__, encoding="utf-8" + ) + + + def generate(*args, **kwargs): + return {} + + + def stale(*args, **kwargs): + return [] + """ +) + + +@pytest.fixture +def mimic(tmp_path, monkeypatch): + """A tree that looks like a skilldeck checkout, whose scripts leave a + sentinel file when imported.""" + root = tmp_path / "fork" + for relative in ("scripts/build_plugin.py", "evals/run_evals.py"): + (root / relative).parent.mkdir(parents=True, exist_ok=True) + (root / relative).write_text(SENTINEL_SCRIPT, encoding="utf-8") + (root / "evals" / "fixtures").mkdir() + (root / "src" / "skilldeck" / "skills").mkdir(parents=True) + (root / "docs").mkdir() + (root / "docs" / "finding-output.md").write_text(FINDING_OUTPUT, encoding="utf-8") + (root / "pyproject.toml").write_text( + '[project]\nname = "skilldeck"\nversion = "0.3.0"\n', encoding="utf-8" + ) + monkeypatch.chdir(root) + _invoke("new", NAME, "--category", "review", "--description", DESCRIPTION) + _complete(root / "src" / "skilldeck" / "skills" / NAME) + return root + + +@pytest.mark.parametrize( + "how", ["cwd", "skills-dir", "path", "cwd inside the skills directory"] +) +def test_validate_never_runs_a_trees_scripts(mimic, monkeypatch, how): + skills = mimic / "src" / "skilldeck" / "skills" + assert authoring.checkout_of(skills) is not None + assert not authoring._trusted_checkout(authoring.Checkout(mimic)) + if how == "cwd": + args = [NAME] + elif how == "skills-dir": + monkeypatch.chdir(mimic.parent) + args = ["--skills-dir", skills] + elif how == "path": + monkeypatch.chdir(mimic.parent) + args = [skills / NAME] + else: + monkeypatch.chdir(skills) + args = [f"./{NAME}"] + report = json.loads( + _runner().invoke(cli, ["validate", "--json", *map(str, args)]).stdout + ) + assert not (mimic / "RAN").exists() + assert not (mimic / "evals" / "RAN").exists() + (skipped,) = [s for s in report["skipped"] if "generated-output" in s["check"]] + assert "uv run --extra dev skilldeck validate" in skipped["reason"] + assert report["skills"][0]["checkout"] is True + # the text-only finding-output check still applies, and passes here + assert "docs.finding-output" not in _rules(report) + assert not _rules(report) & {"generated.stale", "eval.fixture-missing"} diff --git a/tests/test_eval_fixtures.py b/tests/test_eval_fixtures.py index 370545f..b577193 100644 --- a/tests/test_eval_fixtures.py +++ b/tests/test_eval_fixtures.py @@ -630,24 +630,31 @@ def test_fixture_is_well_formed(path): assert (path / "change" / plant.file).is_file(), ( f"plant file {plant.file} not in change/" ) - if not fixture.plants: - # a clean-diff fixture's max-findings is its false-positive tolerance - assert fixture.max_findings <= 2, "keep a clean fixture's tolerance small" + # a clean-diff fixture's max-findings is its false-positive tolerance + assert run_evals.clean_tolerance_problem(fixture) is None @pytest.mark.parametrize("path", FIXTURE_DIRS, ids=lambda p: p.name) def test_plant_keywords_describe_the_defect_not_the_code(path): - # A keyword copied from the planted code (a variable, an event name, a - # CIDR) is satisfied by any report that quotes the line, whether or not it - # identified the defect. Same whole-word matching as the scorer. - fixture = run_evals.load_fixture(path) - for plant in fixture.plants: - code = (path / "change" / plant.file).read_text(encoding="utf-8") - echoed = [k for k in plant.keywords if run_evals.mentions(code, k)] - assert not echoed, ( - f"{plant.file}: keyword(s) {echoed} appear verbatim in the planted " - "code; keywords must describe the defect, not echo the code" - ) + # shared with skilldeck validate; see run_evals.echoed_keywords + problems = run_evals.echoed_keywords(run_evals.load_fixture(path)) + assert not problems, "; ".join(problems) + + +def test_fixture_content_checks_catch_what_they_are_for(tmp_path): + change = tmp_path / "change" + change.mkdir() + (change / "app.py").write_text("session_token = read()\n", encoding="utf-8") + plant = run_evals.Plant(file="app.py", keywords=("session_token", "leak")) + fixture = run_evals.Fixture(tmp_path, "x", (plant,), max_findings=1) + (problem,) = run_evals.echoed_keywords(fixture) + assert "['session_token']" in problem + clean = run_evals.Fixture(tmp_path, "x", (), max_findings=3) + assert "at most 2" in run_evals.clean_tolerance_problem(clean) + assert ( + run_evals.clean_tolerance_problem(dataclasses.replace(clean, max_findings=2)) + is None + ) def test_every_planted_fixture_has_sample_reports(): diff --git a/tests/test_skill_structure.py b/tests/test_skill_structure.py index 011ee77..558ffcd 100644 --- a/tests/test_skill_structure.py +++ b/tests/test_skill_structure.py @@ -16,7 +16,7 @@ import pytest -from skilldeck import lint +from skilldeck import authoring, lint from skilldeck.adapters import ADAPTERS from skilldeck.registry import discover_skills @@ -99,6 +99,66 @@ def test_all_bundled_skills_are_covered(): # --- the rules catch what they are for ------------------------------------------ +# Every required piece, the rule that must fire when it is gone, and how to +# remove it from a body that has it. Listed here rather than read from lint's +# tables, so dropping a table entry fails a test instead of passing silently. +REQUIRED_PIECES = [ + ("Scope heading", "structure.section", "\n## Scope\n", "\n## Where\n"), + ("Output heading", "structure.section", "\n## Output\n", "\n## Report\n"), + ("uncommitted changes", "structure.phrase", "uncommitted", "pending"), + ("worked example", "structure.phrase", "For example:", "Such as:"), + ("verify", "structure.phrase", "Verify before reporting", "Check first"), + ("header", "structure.phrase", "Open the report with one line", "Report"), + ("fetch", "structure.scope", "`git fetch`", "`git pull`"), + ( + "three-dot diff", + "structure.scope", + "`git diff origin/...HEAD`", + "`git diff origin/ HEAD`", + ), + ( + "untracked files", + "structure.scope", + "`git ls-files --others --exclude-standard`", + "`git ls-files`", + ), + ("rubric reference", "structure.output", "shared severity rubric", "rubric"), + ("findings cap", "structure.output", "more than ~10 survive", "many survive"), + ( + "header range", + "structure.output", + "`Reviewed origin/main...HEAD (", + "`Reviewed main (", + ), + ("nothing in scope", "structure.nothing-in-scope", "say so and stop", "end"), +] + + +def _skeleton(): + """A body that has every piece: what skilldeck new writes.""" + return authoring.SKILL_TEMPLATE.format( + title="Widget Review", rubric=lint.SEVERITY_RUBRIC + ) + + +def test_the_skeleton_has_every_piece(): + assert lint.structure_problems("widget-review", _skeleton()) == [] + + +@pytest.mark.parametrize( + ("rule", "old", "new"), + [piece[1:] for piece in REQUIRED_PIECES], + ids=[piece[0] for piece in REQUIRED_PIECES], +) +def test_removing_a_required_piece_fires_its_rule(rule, old, new): + body = _skeleton() + pattern = r"\s+".join(map(re.escape, old.split(" "))) + changed, count = re.subn(pattern, new, body) + assert count, f"the skeleton lacks {old!r}" + rules = {p.rule for p in lint.structure_problems("widget-review", changed)} + assert rule in rules + + _GOOD = SKILLS[0]