diff --git a/CHANGELOG.md b/CHANGELOG.md index e470a5f..2ed5659 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -473,6 +473,43 @@ 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 the read-only review `capabilities` + the bundled review skills declare: read the repository, `git fetch`, + `git diff` and `git ls-files`, and the git remote) 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 and the capability declaration, + the bundle rules (links, directories, undeclared executables and other + files, each reported and never followed), structure, cited sources and + local links, declared commands against the body's code spans, leftover + placeholders, 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). 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, capability 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, citation and declared-command rules moved from the tests into + `skilldeck.lint`, which the tests and `validate` share, and the registry's + errors carry the 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. - Lifecycle and compatibility policy, `docs/lifecycle.md` (#78), linked from the README, `CONTRIBUTING.md`, `docs/releasing.md`, `docs/authoring-skills.md`, `docs/compatibility.md` and `docs/catalog.md`. diff --git a/CLAUDE.md b/CLAUDE.md index 6fa5701..c530c2e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,19 +49,38 @@ 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, and the bundle rules: a skill directory is exactly regular `meta.yaml` + `skill.md` (no scripts, assets, symlinks or junctions; OS/editor leftovers such as `.DS_Store` are ignored when loading but still fail `provenance --verify`), and `skill.md` links only - to the web or its own headings + to the web or its own headings. Each `SkillError` for a malformed skill + carries its `validate` rule id - `capabilities.py` — the versioned `capabilities` declaration every `meta.yaml` carries (files, commands, network, credentials, tools, artifacts), its validation, the `## Declared capabilities` notice adapters append for skills that ask for more than a read-only review (reading files plus read-only git commands; those render unchanged), and the summary `show --summary` / `install --dry-run` print + - `lint.py` — the one home of the skill rules (skill-directory bundle, + structural template, severity rubric copy, citation hygiene, declared + commands vs code spans, 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 with the read-only review + capability baseline, `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. `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; links 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 @@ -133,16 +152,18 @@ 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** - 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). +- 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 - 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/CONTRIBUTING.md b/CONTRIBUTING.md index c3230a7..29da149 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,9 +50,17 @@ create them. 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, and its `capabilities` must declare -what `skill.md` asks the agent to do (commands, network, credentials, edits). 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. +what `skill.md` asks the agent to do (commands, network, credentials, edits). 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. Deprecating or removing a skill, dropping an agent, or making a major version change needs a `CHANGELOG.md` entry that names it, and CI checks for one. See diff --git a/README.md b/README.md index 99d0cc9..63e77ef 100644 --- a/README.md +++ b/README.md @@ -156,6 +156,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 @@ -222,9 +226,13 @@ release; the package remains unpublished today. ## Authoring skills Each skill is a directory under `src/skilldeck/skills/` containing a `meta.yaml` -(including its capability declaration) and a `skill.md`, and nothing else. See -[docs/authoring-skills.md](docs/authoring-skills.md), and -follow the [contributor guide](CONTRIBUTING.md) for setup and validation. +(including its capability declaration) and a `skill.md`, and nothing else. +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 and support diff --git a/docs/authoring-skills.md b/docs/authoring-skills.md index fcbaa76..41ea3a5 100644 --- a/docs/authoring-skills.md +++ b/docs/authoring-skills.md @@ -1,19 +1,295 @@ # 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, agents, capabilities +└── skill.md # the agent-neutral instructions ``` Those two regular files are the whole skill: see [What a skill directory may hold](#what-a-skill-directory-may-hold). +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, declare in meta.yaml's +# capabilities anything it asks beyond the read-only review baseline, +# plant a defect in 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 +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. `validate` runs the eval-fixture and generated-output +checks only then (see [Trust](#trust)). + +## 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`, + `supported-agents` listing every agent unless `--agent` names some, and + the [capabilities](#capabilities) of a read-only review, as the bundled + review skills declare them: read the repository, edit nothing, run + `git fetch`, `git diff` and `git ls-files` (the commands the skeleton's + Scope steps use), and contact the git remote. A skill with that + declaration renders without a `## Declared capabilities` notice. When the + skill you write asks for more (another command, an edit, a credential, an + agent tool, a file it creates), declare it: `validate` checks the + declaration against the body's commands. +- `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`, capabilities included, with the same validation that loads + skills for `install`; +- the [bundle rules](#what-a-skill-directory-may-hold), one problem per + offending entry (a link, a directory, an undeclared executable or any + other file; OS and editor leftovers are ignored, as when loading); +- the `skill.md` structure, cited sources and local links, and its code + spans against `capabilities.commands`: the rules in `skilldeck.lint`, which + the test suite also applies to every bundled skill; +- that no `TODO(author)` placeholder is left; +- 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/`, 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). 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 is followed through a +symlink or junction: a linked skill directory, `meta.yaml` or `skill.md` is +reported (`skill.link`) 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: + +``` +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 +{ + "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. +`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. | +| `meta.capabilities` | error | `capabilities` follows the [capability schema](#capabilities). | +| `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`: no subdirectory or other file. | +| `skill.executable` | error | The skill directory holds no script or program. | +| `skill.link` | error | Neither the skill directory nor anything in it is a symlink or junction. | +| `skill.unreadable` | error | The skill directory and its entries can be read. | +| `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. | +| `references.local-link` | error | `skill.md` links only to web pages and its own headings. | +| `capabilities.undeclared-command` | error | Every command a code span in `skill.md` runs is declared in `capabilities.commands`. | +| `capabilities.unused-command` | error | Every declared command appears in `skill.md`. | +| `capabilities.network` | error | A declared `git fetch` is declared as network use of the git remote. | +| `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.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. | +| `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 @@ -171,14 +447,18 @@ Where the declaration shows up: - **For tools**: `skilldeck catalog --json` reports it as each skill's `capabilities` (see [the skill catalog](catalog.md)). -`tests/test_skill_structure.py` checks the bundled skills' `commands` -against their bodies both ways. A code span that runs a known program -(common package managers, scanners, test runners, network clients, -interpreters, and every program some skill declares), such as +`skilldeck validate` checks a skill's `commands` against its body both ways +(and `tests/test_skill_structure.py` applies the same `skilldeck.lint` rule +to every bundled skill). A code span that runs a known program (common +package managers, scanners, test runners, network clients, interpreters, and +every program a skill in the same directory declares), such as `git diff origin/...HEAD`, must start with a command the skill -declares, and every declared command must appear in the body. A span the -skill only quotes, as a pattern to look for or a command to avoid, is listed -in the test's `MENTIONED_ONLY`. +declares (`capabilities.undeclared-command`), and every declared command +must appear in the body (`capabilities.unused-command`); a declared +`git fetch` must come with network use of the git remote +(`capabilities.network`). A span a bundled skill only quotes, as a pattern +to look for or a command to avoid, is listed in `MENTIONED_ONLY` in +`skilldeck/lint.py`. ### What a skill directory may hold @@ -223,9 +503,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 @@ -245,9 +530,12 @@ skill one line that leaves the owner's area to the owner. ## Testing your skill ```bash -skilldeck list # should show your new skill -skilldeck show my-skill --summary # check the capabilities it declares +skilldeck validate my-skill # every rule, offline +skilldeck list # should show your new skill +skilldeck show my-skill --summary # the capabilities it declares +skilldeck show my-skill --agent claude # the rendered file 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/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 new file mode 100644 index 0000000..afb66f0 --- /dev/null +++ b/src/skilldeck/authoring.py @@ -0,0 +1,1026 @@ +"""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 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 hashlib +import importlib.util +import os +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 .capabilities import CAPABILITY_SCHEMA +from .catalog import skill_entry +from .lint import ( + ERROR, + INCOMPLETE, + PLACEHOLDER, + SEVERITY_RUBRIC, + SKILL_FILES, + Problem, + bundle_problems, + command_problems, + command_programs, + description_problems, + finding_output_problems, + placeholder_problems, + reference_problems, + structure_problems, +) +from .registry import ( + MAX_DESCRIPTION_LENGTH, + MAX_NAME_LENGTH, + NAME_RE, + Skill, + SkillError, + SkillMeta, + check_meta, + discover_skills, + is_junk, + is_link, + link_kind, + 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) +#: 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. + + 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: + return absolute.as_posix() + + +def skill_dirs(skills_dir: Path) -> list[Path]: + """The skill directories in ``skills_dir``, as discovery finds them, plus + any link discovery would refuse (validate reports it).""" + return sorted( + child + for child in skills_dir.iterdir() + if not child.name.startswith(".") + and not is_junk(child.name) + and (is_link(child) or child.is_dir()) + ) + + +# -- 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("-")) + + +#: what a new skill declares: the read-only review baseline the bundled +#: review skills use, matching the skeleton's Scope steps (read the +#: repository; git fetch, git diff and git ls-files; the git remote), so it +#: renders without a ``## Declared capabilities`` notice +REVIEW_CAPABILITIES: dict[str, object] = { + "schema": CAPABILITY_SCHEMA, + "files": {"read": "repo", "write": "none"}, + "commands": ["git fetch", "git diff", "git ls-files"], + "network": ["the git remote, via git fetch, to bring the base branch up to date"], + "credentials": [], + "tools": [], + "artifacts": [], +} + + +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), + "capabilities": REVIEW_CAPABILITIES, + } + 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 it is in a checkout's skills directory (repository checks) + 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) + + @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 + ], + } + + +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 trusted checkout's script (``evals/run_evals.py``, + ``scripts/build_plugin.py``) by path, once per process. + + 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: + 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" + + +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.""" + + 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): + if is_link(child): + continue + 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, trusted: bool + ) -> 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] = [] + + # A link is reported and never followed: its target's path and + # contents must not reach the report. The skill directory itself + # being one stops everything; a link (or directory) in place of + # meta.yaml or skill.md stops that file being read. + if is_link(skill_dir): + return [ + Problem( + self.show(skill_dir), + "skill.link", + f"the skill directory is a {link_kind(skill_dir)}", + None, + name, + ) + ], [ + Skipped( + f"every other check of {name}", + "validate does not follow a linked skill directory", + ) + ] + bundle = bundle_problems(skill_dir, self.show) if skill_dir.is_dir() else [] + problems += bundle + # meta.yaml or skill.md that is a link, directory or special file + shown = {self.show(skill_dir / file): file for file in SKILL_FILES} + blocked = {shown[p.path] for p in bundle if p.path in shown} + + meta: SkillMeta | None = None + if "meta.yaml" not in blocked: + try: + meta = check_meta(skill_dir, set(ADAPTERS)) + except SkillError as exc: + problems.append( + Problem( + self.show(exc.file or meta_path), + exc.rule or "meta.syntax", + exc.detail, + exc.line, + name, + ) + ) + meta_text = _read_text(meta_path) + if meta_text is not None: + problems += placeholder_problems(meta_text, self.show(meta_path), name) + + body: str | None = None + if "skill.md" not in blocked: + if not body_path.is_file(): + problems.append( + Problem( + self.show(body_path), + "body.missing", + "missing skill.md", + None, + name, + ) + ) + else: + body = _read_text(body_path) + if body is None: + problems.append( + Problem( + self.show(body_path), + "body.encoding", + "skill.md is not valid UTF-8", + None, + 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) + if meta is not None: + programs = command_programs( + [ + meta.capabilities, + *(s.capabilities for s in self.siblings(skill_dir.parent)), + ] + ) + problems += command_problems( + name, + body, + meta.capabilities, + programs, + where, + self.show(meta_path), + ) + + # the bundle and local-link rules are reported above, so the skill is + # rendered whenever its two files load + skill = meta.skill(body, skill_dir) if meta and body is not None else None + 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 == meta_path + ] + + if checkout is not None: + doc = checkout.root / FINDING_OUTPUT_DOC + 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]: + """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, + 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 = ( + [ + 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, + ) + ) + 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 file.is_symlink() or not file.is_file(): + continue + if "__pycache__" in file.parts: + continue + 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) + 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; the checkout must + be trusted (see :func:`_trusted_checkout`).""" + 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. 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(Path(os.path.abspath(base or Path.cwd()))) + report = Report() + checkouts: dict[Path, Checkout] = {} + outside: set[str] = set() + seen: set[Path] = set() + trusted: dict[Path, bool] = {} + for path in skill_paths: + # 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.resolve()) + checkout = checkout_of(skill_dir.parent) + if checkout is None: + outside.add(checker.show(skill_dir.parent)) + else: + checkouts[checkout.root] = 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( + 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 {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 + 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 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}") + 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 0a82836..19e93b5 100644 --- a/src/skilldeck/catalog.py +++ b/src/skilldeck/catalog.py @@ -103,6 +103,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 ed47f30..693b34f 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 @@ -9,6 +10,7 @@ import click +from . import authoring from .adapters import ( ADAPTERS, ALL_ADAPTERS, @@ -25,6 +27,7 @@ catalog_schema_text, filter_catalog, ) +from .lint import PLACEHOLDER from .provenance import ( REPOSITORY_URL, canonical_json, @@ -942,6 +945,203 @@ 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"{shlex.quote(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.", + "Declare in meta.yaml's capabilities anything the skill asks beyond " + "the read-only review it declares now (another command, an edit, a " + "credential, an agent tool, a file it creates).", + ] + if in_checkout: + steps += [ + f"Build the eval fixture in evals/fixtures/{name}/ (see " + "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}") + + +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 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(): + 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..4d9b7bc --- /dev/null +++ b/src/skilldeck/lint.py @@ -0,0 +1,856 @@ +"""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, the declared-commands check and the placeholder check are +defined once, here. The bundle rules (what a skill directory may hold) and +the capability schema stay in :mod:`skilldeck.registry` and +:mod:`skilldeck.capabilities`, which loading enforces; this module maps +their findings to rule ids. + +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 collections.abc import Callable, Collection, Iterable +from dataclasses import dataclass +from pathlib import Path + +from . import registry +from .capabilities import Capabilities + +#: 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" + +#: the only files a skill directory holds +SKILL_FILES = registry.BUNDLE_FILES + + +@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", + ), + "meta.capabilities": Rule( + ERROR, + "capabilities follows the capability schema", + "fix the capabilities block as the message says; see " + f"{_AUTHORING}#capabilities", + ), + # 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", + ), + "skill.executable": Rule( + ERROR, + "the skill directory holds no script or program", + "remove it: a skill cannot ship a script. Declare a program on PATH " + "in capabilities.commands, or a for a command the " + f"project defines ({_AUTHORING}#capabilities)", + ), + "skill.link": Rule( + ERROR, + "neither the skill directory nor anything in it is a symlink or junction", + "replace the link with a regular file or directory; validate never " + "follows one, so what it points at is not checked at all", + ), + "skill.unreadable": Rule( + ERROR, + "the skill directory and its entries can be read", + "fix the permissions so the skill directory can be listed and read", + ), + "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", + ), + "references.local-link": Rule( + ERROR, + "skill.md links only to web pages and its own headings", + "link to a web page (http, https, mailto) or a #heading: a skill " + "ships only meta.yaml and skill.md, so a relative or file link has " + "nothing to resolve to once installed", + ), + "capabilities.undeclared-command": Rule( + ERROR, + "every command a code span in skill.md runs is declared", + "declare the command in capabilities.commands, or, for a span the " + "skill only quotes (a pattern to look for, a command to avoid), " + "rephrase it; a bundled skill can list it in MENTIONED_ONLY in " + "skilldeck/lint.py", + ), + "capabilities.unused-command": Rule( + ERROR, + "every declared command appears in skill.md", + "remove the command from capabilities.commands, or name it in " + "skill.md where the skill asks the agent to run it", + ), + "capabilities.network": Rule( + ERROR, + "a declared git fetch is declared as network use of the git remote", + "add a capabilities.network entry such as: the git remote, via git " + "fetch, to bring the base branch up to date", + ), + "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.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/", + "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 skill directory ---------------------------------------------------------- + + +#: the rule each kind of :class:`registry.BundleEntry` breaks +_BUNDLE_RULES = { + "link": "skill.link", + "executable": "skill.executable", + "unreadable": "skill.unreadable", + "directory": "skill.unexpected-file", + "special": "skill.unexpected-file", + "extra": "skill.unexpected-file", +} + + +def bundle_problems( + skill_dir: Path, show: Callable[[Path], str] = Path.as_posix +) -> list[Problem]: + """The registry's bundle rules (:func:`registry.bundle_entries`), one + problem per offending entry: a link, a directory or other non-regular + file, an undeclared executable, or any other file. OS and editor + leftovers are ignored, as when loading. + + Nothing is read, and a link 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. + """ + return [ + Problem( + show(skill_dir / entry.name) if entry.name else show(skill_dir), + _BUNDLE_RULES[entry.kind], + entry.message, + skill=skill_dir.name, + ) + for entry in registry.bundle_entries(skill_dir) + ] + + +# -- 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, + ) + # 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})") + 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, + ) + ) + local = registry.local_links(body) + if local: + problems.append( + Problem( + path, + "references.local-link", + f"links to {', '.join(local)}, which the skill cannot ship", + _first_line(body, local[0]), + 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 + + +def _first_line(text: str, needle: str) -> int | None: + index = text.find(needle) + return _line_at(text, index) if index >= 0 else None + + +# -- declared commands -------------------------------------------------------------- + +# Programs a code span in a skill body is taken to run: common CLIs (forges, +# network clients, package managers, scanners, test runners, interpreters and +# infrastructure tools). Every program a skill in the same directory declares +# counts too (see command_programs). A span that is only the program's name is +# a mention, not a command. +KNOWN_PROGRAMS = frozenset( + { + # version control, forges and the network + "git", "gh", "glab", "curl", "wget", + # package managers + "npm", "npx", "pnpm", "yarn", "bun", "pip", "pip3", "pipx", "uv", "uvx", + "poetry", "cargo", "go", "gem", "bundle", "composer", "mvn", "gradle", + "dotnet", "brew", "apt", "apt-get", + # scanners and linters + "pip-audit", "osv-scanner", "govulncheck", "semgrep", "trivy", "grype", + "syft", "checkov", "tfsec", "kics", "kube-score", "conftest", "zizmor", + "actionlint", "bandit", "gitleaks", "trufflehog", "snyk", "safety", + "squawk", "hadolint", + # test runners and build tools + "pytest", "tox", "nox", "jest", "vitest", "mocha", "rspec", "phpunit", + "make", + # interpreters and shells + "python", "python3", "node", "deno", "ruby", "perl", "php", "sh", "bash", + "zsh", "pwsh", + # infrastructure + "docker", "kubectl", "helm", "terraform", + } +) # fmt: skip +# Code spans a bundled skill quotes without asking the agent to run them, per +# skill: patterns to look for in the code under review, or commands to avoid. +# Each must still appear in that skill's body (a test checks). +MENTIONED_ONLY: dict[str, frozenset[str]] = { + "ci-workflow-review": frozenset( + { + # interpreter flags a CI step can inject through + "bash -c", + "node -e", + "perl -e", + "python -c", + "ruby -e", + "sh -c", + 'sh -c "… $VAR"', + 'sh -c \'notify "$1"\' _ "$CI_COMMIT_TITLE"', + } + ), + # the unsafe invocations the skill warns against + "dependency-review": frozenset({"pip install -r", "pip-audit -r "}), +} +_CODE_SPAN_RE = re.compile(r"(? frozenset[str]: + """:data:`KNOWN_PROGRAMS` plus every program ``declarations`` declare + (```` commands aside).""" + return KNOWN_PROGRAMS | { + command.split(" ")[0] + for capabilities in declarations + for command in capabilities.commands + if not command.startswith("<") + } + + +def code_spans(body: str) -> list[tuple[str, int]]: + """Each code span in ``body``, whitespace-normalized, with its line.""" + body = _lf(body) + return [ + (" ".join(match.group(2).split()), _line_at(body, match.start())) + for match in _CODE_SPAN_RE.finditer(body) + ] + + +def _runs(span: str, command: str) -> bool: + return span == command or span.startswith(command + " ") + + +def undeclared_commands( + name: str, body: str, capabilities: Capabilities, programs: Collection[str] +) -> list[str]: + """Code spans in skill ``name``'s body that run one of ``programs`` with a + command ``capabilities`` doesn't declare, sorted, each once.""" + mentioned = MENTIONED_ONLY.get(name, frozenset()) + return sorted( + { + span + for span, _ in code_spans(body) + if span.split(" ")[0] in programs + and span not in programs + and span not in mentioned + and not any(_runs(span, command) for command in capabilities.commands) + } + ) + + +def unused_commands(body: str, capabilities: Capabilities) -> list[str]: + """Declared commands the body never names (```` aside).""" + spans = [span for span, _ in code_spans(body)] + return [ + command + for command in capabilities.commands + if not command.startswith("<") + and not any( + _runs(span, command) or span == command.split(" ")[0] for span in spans + ) + ] + + +def command_problems( + name: str, + body: str, + capabilities: Capabilities, + programs: Collection[str] = KNOWN_PROGRAMS, + path: str = "skill.md", + meta_path: str = "meta.yaml", +) -> list[Problem]: + """Whether ``capabilities.commands`` keeps up with the body, both ways. + + A code span that runs one of ``programs`` (see :func:`command_programs`), + such as ``git diff origin/...HEAD``, must start with a declared + command, unless the skill only quotes it (:data:`MENTIONED_ONLY`); every + declared command must appear in the body; and a declared ``git fetch`` + must come with network use of the git remote. + """ + programs = set(programs) | command_programs([capabilities]) + lines: dict[str, int] = {} + for span, line in code_spans(body): + lines.setdefault(span, line) + problems = [ + Problem( + path, + "capabilities.undeclared-command", + f"`{span}` runs {span.split(' ')[0]}, but capabilities.commands " + "declares no command it starts with", + lines.get(span), + name, + ) + for span in undeclared_commands(name, body, capabilities, programs) + ] + problems += [ + Problem( + meta_path, + "capabilities.unused-command", + f"capabilities.commands declares `{command}`, which skill.md never names", + skill=name, + ) + for command in unused_commands(body, capabilities) + ] + if "git fetch" in capabilities.commands and not any( + "git remote" in entry for entry in capabilities.network + ): + problems.append( + Problem( + meta_path, + "capabilities.network", + "capabilities.commands declares git fetch, but capabilities." + "network has no entry for the git remote it contacts", + skill=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 b53c9c2..20aa7ab 100644 --- a/src/skilldeck/registry.py +++ b/src/skilldeck/registry.py @@ -109,7 +109,38 @@ class SkillError(Exception): - """Raised when a skill directory is malformed.""" + """Raised when a skill directory is malformed. + + For a malformed skill, ``rule`` names the rule it breaks (e.g. + ``meta.version``; ``skilldeck validate`` lists them), ``file`` is the file + 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__( + self, + message: str, + *, + 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( + skill_dir: Path, rule: str, detail: str, file: str = "meta.yaml" +) -> SkillError: + """A :class:`SkillError` for ``skill_dir`` breaking ``rule``.""" + return SkillError( + f"{skill_dir}: {detail}", rule=rule, file=skill_dir / file, detail=detail + ) @dataclass(frozen=True) @@ -135,6 +166,33 @@ class Skill: capabilities: Capabilities = Capabilities() +@dataclass(frozen=True) +class SkillMeta: + """A skill's validated ``meta.yaml``.""" + + name: str + description: str + category: str + version: str + supported_agents: tuple[str, ...] + deprecated: Deprecation | None + capabilities: Capabilities + + def skill(self, body: str, path: Path) -> Skill: + """The skill these fields describe, with ``body`` as its ``skill.md``.""" + return Skill( + name=self.name, + description=self.description, + category=self.category, + version=self.version, + supported_agents=self.supported_agents, + body=body, + path=path, + deprecated=self.deprecated, + capabilities=self.capabilities, + ) + + def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> Skill: """Load and validate a single skill directory. @@ -147,10 +205,14 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> if is_link(skill_dir): raise SkillError( - f"{skill_dir}: the skill directory is a {link_kind(skill_dir)}" + f"{skill_dir}: the skill directory is a {link_kind(skill_dir)}", + rule="skill.link", + file=skill_dir, + detail=f"the skill directory is a {link_kind(skill_dir)}", ) problems = bundle_problems(skill_dir) if skill_dir.is_dir() else [] if problems: + # several rules at once; validate reports each (see bundle_entries) raise SkillError( f"{skill_dir}: {'; '.join(problems)}. A skill directory holds only " "meta.yaml and skill.md, as regular files: skilldeck installs one " @@ -158,39 +220,99 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> "assets or links" ) 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") + + 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 + missing_assets = local_links(body) + if missing_assets: + raise _invalid( + skill_dir, + "references.local-link", + f"skill.md links to {', '.join(missing_assets)}, which " + "the skill cannot ship: a skill is only meta.yaml and skill.md, so " + "a relative or file link has nothing to resolve to once installed. " + f"Link to a web page ({', '.join(WEB_SCHEMES)}) or a #heading instead", + "skill.md", + ) + + return meta.skill(body, skill_dir) + + +def check_meta( + skill_dir: Path, known_agents: Collection[str] | None = None +) -> SkillMeta: + """Validate ``skill_dir/meta.yaml`` alone, as :func:`load_skill` does. + + For ``skilldeck validate``, which checks the skill directory and + ``skill.md`` separately. 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") + return _load_meta(skill_dir, known_agents) + + +def _load_meta(skill_dir: Path, known_agents: Collection[str] | None) -> SkillMeta: + """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: - 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 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 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") @@ -198,14 +320,18 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> # double-quoted escapes such as "\u2028" 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") @@ -214,22 +340,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 = ( @@ -241,34 +377,35 @@ def load_skill(skill_dir: Path, known_agents: Collection[str] | None = None) -> try: capabilities = parse_capabilities(meta["capabilities"]) except CapabilityError as exc: - raise SkillError(f"{skill_dir}: meta.yaml {exc}") from exc + raise _invalid(skill_dir, "meta.capabilities", f"meta.yaml {exc}") from exc - 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 - missing_assets = local_links(body) - if missing_assets: - raise SkillError( - f"{skill_dir}: skill.md links to {', '.join(missing_assets)}, which " - "the skill cannot ship: a skill is only meta.yaml and skill.md, so " - "a relative or file link has nothing to resolve to once installed. " - f"Link to a web page ({', '.join(WEB_SCHEMES)}) or a #heading instead" - ) - - return Skill( + return SkillMeta( name=name, description=description, category=category, version=raw_version, supported_agents=tuple(agents), - body=body, - path=skill_dir, deprecated=deprecated, capabilities=capabilities, ) +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 is_junk(name: str) -> bool: """Whether ``name`` is a file an OS or editor leaves behind, which loading a skill ignores (``provenance --verify`` does not).""" @@ -286,46 +423,79 @@ def link_kind(path: Path) -> str: return "symlink" if path.is_symlink() else "junction" -def bundle_problems(skill_dir: Path) -> list[str]: +@dataclass(frozen=True) +class BundleEntry: + """One entry of a skill directory that breaks the bundle rules.""" + + #: the entry's file name; empty when the directory itself can't be listed + name: str + #: unreadable, link, directory, special, executable or extra + kind: str + message: str + + +def bundle_entries(skill_dir: Path) -> list[BundleEntry]: """What in ``skill_dir`` breaks the bundle rules; ``[]`` if nothing does. - One message per offending entry: a symlink or junction (never followed), - a directory or other non-regular file, or any file besides - :data:`BUNDLE_FILES`, which the message calls executable when its suffix, - execute bit (not on Windows, which has none) or first bytes say it is a - program. Every such file is refused, since a skill cannot ship or declare - one. OS and editor leftovers (:func:`is_junk`) are skipped, unless one is - a link: only an Emacs lock file (``.#name``) is a symlink by nature. + One entry per offending file: a symlink or junction (never followed), a + directory or other non-regular file, or any file besides + :data:`BUNDLE_FILES`, which is ``executable`` when its suffix, execute bit + (not on Windows, which has none) or first bytes say it is a program. + Every such file is refused, since a skill cannot ship or declare one. OS + and editor leftovers (:func:`is_junk`) are skipped, unless one is a link: + only an Emacs lock file (``.#name``) is a symlink by nature. """ try: entries = sorted(skill_dir.iterdir(), key=lambda entry: entry.name) except OSError as exc: - return [f"cannot list the skill directory: {exc}"] - problems: list[str] = [] + return [ + BundleEntry("", "unreadable", f"cannot list the skill directory: {exc}") + ] + problems: list[BundleEntry] = [] for entry in entries: + name = entry.name try: mode = entry.lstat().st_mode except OSError as exc: - problems.append(f"cannot inspect {entry.name}: {exc}") + problems.append( + BundleEntry(name, "unreadable", f"cannot inspect {name}: {exc}") + ) continue link = stat.S_ISLNK(mode) or is_link(entry) - if is_junk(entry.name) and (not link or entry.name.startswith(".#")): + if is_junk(name) and (not link or name.startswith(".#")): continue if link: - problems.append(f"{entry.name} is a {link_kind(entry)}") + problems.append( + BundleEntry(name, "link", f"{name} is a {link_kind(entry)}") + ) elif stat.S_ISDIR(mode): - problems.append(f"{entry.name} is a directory") + problems.append(BundleEntry(name, "directory", f"{name} is a directory")) elif not stat.S_ISREG(mode): - problems.append(f"{entry.name} is not a regular file") - elif entry.name not in BUNDLE_FILES: + problems.append( + BundleEntry(name, "special", f"{name} is not a regular file") + ) + elif name not in BUNDLE_FILES: why = _executable(entry, mode) if why: - problems.append(f"{entry.name} is an undeclared executable ({why})") + problems.append( + BundleEntry( + name, + "executable", + f"{name} is an undeclared executable ({why})", + ) + ) else: - problems.append(f"{entry.name} is not meta.yaml or skill.md") + problems.append( + BundleEntry(name, "extra", f"{name} is not meta.yaml or skill.md") + ) return problems +def bundle_problems(skill_dir: Path) -> list[str]: + """The message of each :func:`bundle_entries` entry; ``[]`` if none.""" + return [entry.message for entry in bundle_entries(skill_dir)] + + def _executable(path: Path, mode: int) -> str | None: """Why the regular file at ``path`` is a program; None if nothing says so.""" suffix = path.suffix.lower() @@ -421,24 +591,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 @@ -461,25 +637,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"] @@ -489,25 +670,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) @@ -529,44 +718,52 @@ def discover_skills( raise SkillError(f"{child}: the skill directory is a {link_kind(child)}") if child.is_dir(): skills.append(load_skill(child, known_agents)) - _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..d7dd4c2 --- /dev/null +++ b/tests/test_authoring.py @@ -0,0 +1,917 @@ +"""``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_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, + 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_new_declares_the_read_only_review_baseline(skills_dir): + # the same declaration, word for word, as the bundled review skills + _new(skills_dir) + skill = registry.load_skill(skills_dir / NAME, set(ADAPTERS)) + resilience = registry.load_skill( + registry.DEFAULT_SKILLS_DIR / "resilience-review", set(ADAPTERS) + ) + assert skill.capabilities == resilience.capabilities + assert not skill.capabilities.beyond_review_baseline + # so no adapter adds a Declared capabilities notice, and the skeleton's + # own commands match its declaration both ways + for adapter in ALL_ADAPTERS.values(): + if adapter.supports(skill): + assert "## Declared capabilities" not in adapter.render(skill) + assert lint.command_problems(skill.name, skill.body, skill.capabilities) == [] + + +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: 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): + _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_capability_schema_error(completed): + _edit(completed / "meta.yaml", " read: repo\n", " read: everything\n") + problem = _problem(_report("--skills-dir", completed.parent), "meta.capabilities") + assert problem["path"] == f"skills/{NAME}/meta.yaml" + assert ( + "capabilities.files.read must be one of none, diff, repo" + in (problem["message"]) + ) + + +def test_undeclared_command(completed): + _edit(completed / "skill.md", SOURCE, SOURCE + " Then run `npm audit --json`.") + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "capabilities.undeclared-command") + assert problem["path"] == f"skills/{NAME}/skill.md" + assert "`npm audit --json`" in problem["message"] + text = (completed / "skill.md").read_text(encoding="utf-8") + assert problem["line"] == text[: text.index("npm audit")].count("\n") + 1 + + +def test_unused_command_and_undeclared_remote(completed): + _edit( + completed / "meta.yaml", + " - git ls-files\n", + " - git ls-files\n - npm audit\n", + ) + _edit( + completed / "meta.yaml", + " - the git remote, via git fetch, to bring the base branch up to date\n", + " []\n", + ) + meta = (completed / "meta.yaml").read_text(encoding="utf-8") + (completed / "meta.yaml").write_text( + meta.replace(" network:\n []\n", " network: []\n"), encoding="utf-8" + ) + report = _report("--skills-dir", completed.parent) + assert "`npm audit`" in _problem(report, "capabilities.unused-command")["message"] + assert ( + _problem(report, "capabilities.network")["path"] == f"skills/{NAME}/meta.yaml" + ) + + +def test_local_link(completed): + _edit(completed / "skill.md", SOURCE, SOURCE + " See [notes](notes.md).") + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "references.local-link") + assert "notes.md" in problem["message"] and problem["line"] + # reported on its own: the skill still renders, so nothing else hides + assert not [s for s in report["skipped"] if "rendering" in s["check"]] + + +@pytest.mark.parametrize( + ("name", "content", "rule"), + [ + ("run.sh", b"echo hi\n", "skill.executable"), + ("helper", b"#!/bin/sh\necho hi\n", "skill.executable"), + ("notes.txt", b"scratch\n", "skill.unexpected-file"), + ], +) +def test_bundle_rules(completed, name, content, rule): + (completed / name).write_bytes(content) + problem = _problem(_report("--skills-dir", completed.parent), rule) + assert problem["path"] == f"skills/{NAME}/{name}" + + +def test_bundle_subdirectory(completed): + (completed / "assets").mkdir() + problem = _problem( + _report("--skills-dir", completed.parent), "skill.unexpected-file" + ) + assert problem["message"] == "assets is a directory" + + +def test_os_and_editor_leftovers_are_ignored(completed): + # as when loading: one stray file doesn't break validation + for junk in (".DS_Store", "skill.md~", ".skill.md.swp"): + (completed / junk).write_bytes(b"\x00junk") + (completed.parent / "Thumbs.db").write_bytes(b"\x00") + out = _invoke("validate", "--skills-dir", completed.parent).stdout + assert f"{NAME}: ok" in out + + +def test_linked_skill_directory_is_reported_and_not_followed( + completed, tmp_path, symlink +): + elsewhere = tmp_path / "elsewhere" / "linked-review" + elsewhere.parent.mkdir() + shutil.copytree(completed, elsewhere) + symlink(completed.parent / "linked-review", elsewhere) + report = _report("--skills-dir", completed.parent) + problem = _problem(report, "skill.link") + assert problem["path"] == "skills/linked-review" + assert [p["rule"] for p in report["problems"]] == ["skill.link"] + assert "elsewhere" not in json.dumps(report) + + +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 + + +@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 + + +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) + # 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 + + +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 + 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 + 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_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 + _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) + + +# --- 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.link") + 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_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 e2a7c4c..00fab8f 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 @@ -15,6 +17,7 @@ import pytest +from skilldeck import authoring, lint from skilldeck.adapters import ADAPTERS from skilldeck.registry import discover_skills @@ -25,96 +28,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(): @@ -127,7 +79,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" ) @@ -136,135 +88,51 @@ 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}" + problems = [p for s in SKILLS for p in lint.finding_output_problems(doc, s.name)] + assert not problems, _explain(problems) -@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" - ) - - -# Programs a code span in a skill body is taken to run: common CLIs (forges, -# network clients, package managers, scanners, test runners, interpreters and -# infrastructure tools) and every program an official skill declares. A span -# that is only the program's name is a mention, not a command. -KNOWN_PROGRAMS = { - # version control, forges and the network - "git", "gh", "glab", "curl", "wget", - # package managers - "npm", "npx", "pnpm", "yarn", "bun", "pip", "pip3", "pipx", "uv", "uvx", - "poetry", "cargo", "go", "gem", "bundle", "composer", "mvn", "gradle", - "dotnet", "brew", "apt", "apt-get", - # scanners and linters - "pip-audit", "osv-scanner", "govulncheck", "semgrep", "trivy", "grype", - "syft", "checkov", "tfsec", "kics", "kube-score", "conftest", "zizmor", - "actionlint", "bandit", "gitleaks", "trufflehog", "snyk", "safety", - "squawk", "hadolint", - # test runners and build tools - "pytest", "tox", "nox", "jest", "vitest", "mocha", "rspec", "phpunit", "make", - # interpreters and shells - "python", "python3", "node", "deno", "ruby", "perl", "php", "sh", "bash", - "zsh", "pwsh", - # infrastructure - "docker", "kubectl", "helm", "terraform", -} # fmt: skip -COMMAND_PROGRAMS = KNOWN_PROGRAMS | { - command.split(" ")[0] - for skill in SKILLS - for command in skill.capabilities.commands - if not command.startswith("<") -} -# Code spans a skill quotes without asking the agent to run them, per skill: -# patterns to look for in the code under review, or commands to avoid. Each -# must still appear in that skill's body. -MENTIONED_ONLY = { - "ci-workflow-review": { - # interpreter flags a CI step can inject through - "bash -c", - "node -e", - "perl -e", - "python -c", - "ruby -e", - "sh -c", - 'sh -c "… $VAR"', - 'sh -c \'notify "$1"\' _ "$CI_COMMIT_TITLE"', - }, - # the unsafe invocations the skill warns against - "dependency-review": {"pip install -r", "pip-audit -r "}, -} -_CODE_SPAN_RE = re.compile(r"(?= 7 + + +# --- 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] + + +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)