From 6e1e1e951102119f47e549b9adbe008a5482ddc3 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Wed, 21 Oct 2026 21:47:35 -0700 Subject: [PATCH 01/11] feat(catalog): review skills against the spec and flag identical copies `catalog scan` checked only that a skill loads. Each loaded skill is now also reviewed: the directory name must equal `name` and be kebab-case (at most 64 characters), and `description` must be at most 1024 characters (errors, exit 1). A missing `name` and a SKILL.md byte-identical to an earlier skill's are warnings naming the first copy. disable-model-invocation is shown as a note. `--format json` prints one object with a row per skill and task. Signed-off-by: Nikolay Petrov --- ...50000_napetrov_catalog-scan-spec-review.md | 20 ++ dokimi/docs/external-content.md | 10 + dokimi/dokimi/commands/catalog.py | 76 ++++++-- dokimi/dokimi/external/__init__.py | 4 + dokimi/dokimi/external/validate.py | 124 +++++++++++++ dokimi/tests/test_catalog_validate.py | 173 ++++++++++++++++++ 6 files changed, 396 insertions(+), 11 deletions(-) create mode 100644 dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md create mode 100644 dokimi/dokimi/external/validate.py create mode 100644 dokimi/tests/test_catalog_validate.py diff --git a/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md new file mode 100644 index 000000000..bc3a863b2 --- /dev/null +++ b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md @@ -0,0 +1,20 @@ +### Added + +- **`catalog scan` reviews each skill against the Agent Skills spec** — a loaded skill + whose `name` is not its directory name or not kebab-case (at most 64 characters), or + whose `description` is over 1024 characters, is an `error`; a `name` missing from the + frontmatter, or a `SKILL.md` byte-identical to an earlier skill's (a mirror, named + with its first copy), is a `warn`. `disable-model-invocation: true` is printed as a + note, since the model is never offered that skill. Each finding is printed under its + skill. +- **`catalog scan --format json`** — one object with a row per skill (`state` is + `ok`, `warn` or `error`, `findings`, `content_sha256`, `duplicate_of`) and per task + (`state` is `ok`, `skipped` or `error`), for a CI gate or a script. + +### Changed + +- **`catalog scan` exits 1 on a skill spec error** (a name that is not the directory + name or not kebab-case, a description over 1024 characters), as it already did for a + skill that does not load. Warnings (mirrors, a missing `name`) do not change the exit + code. A catalog that declares such a skill now fails the scan; fix the frontmatter + or drop the skill from the scan. diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index 7c0951550..efc5aac9b 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -115,6 +115,16 @@ with no directory is skipped, while a `planned` *skill* with no directory is an error — the skill reader has no notion of a not-yet-written entry. Either drop such a skill from `skills.yaml` or expect the non-zero exit. +A loaded skill is also checked against the Agent Skills spec. Its `name` must be +its directory name and kebab-case (at most 64 characters), and its `description` +at most 1024 characters: a violation is an `error` and exits 1, as a load error +does. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to an +earlier skill's, is a `warn` that names the first copy; warnings do not change +the exit code. `disable-model-invocation: true` is printed as a `note`, because +the model is never offered that skill. Each finding is printed under its skill. +`--format json` prints the same rows as one object (`state`, `findings`, +`content_sha256`, `duplicate_of`) for a script or a CI gate. + Restrict the scan with `--kind`: ```console diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index 30a2be7c7..270730332 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -13,17 +13,41 @@ from __future__ import annotations import argparse +import json import sys from pathlib import Path +def _skill_row(skill, review) -> dict: + """One skill of the scan report. ``state`` is ``error`` for a load error or an error finding, + ``warn`` for a warning finding only, else ``ok``.""" + if not skill.loaded: + state, findings = "error", [] + else: + state, findings = review.state, review.findings + return { + "name": skill.name, + "manifest_status": skill.status, + "path": str(skill.path) if skill.path else None, + "state": state, + "error": skill.error, + "disable_model_invocation": bool(review and review.disable_model_invocation), + "content_sha256": review.content_sha256 if review else None, + "duplicate_of": review.duplicate_of if review else None, + "findings": [{"level": f.level, "code": f.code, "message": f.message} for f in findings], + } + + def cmd_catalog_scan(args: argparse.Namespace) -> None: from dokimi.external import read_skill_manifest, read_task_manifest + from dokimi.external.validate import review_skills root = Path(args.root) if not root.is_dir(): raise SystemExit(f"--root is not a directory: {root}") + as_json = args.format == "json" + report: dict = {"root": str(root), "skills": [], "tasks": []} any_failure = False if args.kind in ("skills", "all"): @@ -31,21 +55,39 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: skills = read_skill_manifest(root) except ValueError as exc: raise SystemExit(f"skills.yaml: {exc}") from exc - print(f"# skills ({len(skills)} declared under {root})") - for skill in skills: - state = "ok" if skill.loaded else f"ERROR: {skill.error}" - status = skill.status or "-" - print(f"{skill.name}\t{status}\t{skill.path}\t{state}") - if not skill.loaded: + reviews = review_skills(skills) + counts = {"ok": 0, "warn": 0, "error": 0} + for skill, review in zip(skills, reviews, strict=True): + row = _skill_row(skill, review) + counts[row["state"]] += 1 + report["skills"].append(row) + if row["state"] == "error": any_failure = True - print() + if not as_json: + print(f"# skills ({len(skills)} declared under {root}; " + f"{counts['ok']} ok, {counts['warn']} warn, {counts['error']} error)") + for row in report["skills"]: + if row["error"] is not None: + state = f"ERROR: {row['error']}" + elif row["state"] == "ok": + state = "ok" + else: + worst = next(f for f in row["findings"] if f["level"] == row["state"]) + state = f"{'ERROR' if row['state'] == 'error' else 'warn'}: {worst['message']}" + print(f"{row['name']}\t{row['manifest_status'] or '-'}\t{row['path'] or '-'}\t{state}") + for f in row["findings"]: + print(f" {f['level']} {f['code']}: {f['message']}") + if row["disable_model_invocation"]: + print(" note disable-model-invocation: true (the model is never offered this skill)") + print() if args.kind in ("tasks", "all"): try: tasks = read_task_manifest(root, tasks_subdir=args.tasks_subdir) except ValueError as exc: raise SystemExit(f"suites.json: {exc}") from exc - print(f"# tasks ({len(tasks)} declared under {root})") + if not as_json: + print(f"# tasks ({len(tasks)} declared under {root})") for task in tasks: if task.loaded: state = "ok" @@ -54,9 +96,17 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: else: state = f"ERROR: {task.reason}" any_failure = True - track = task.track or "-" - status = task.status or "-" - print(f"{task.name}\t{status}\t{track}\t{state}") + report["tasks"].append({ + "name": task.name, "manifest_status": task.status, "track": task.track, + "path": str(task.path) if task.path else None, + "state": "ok" if task.loaded else "skipped" if task.skipped else "error", + "reason": task.reason, + }) + if not as_json: + print(f"{task.name}\t{task.status or '-'}\t{task.track or '-'}\t{state}") + + if as_json: + print(json.dumps(report, indent=2)) if any_failure: raise SystemExit(1) @@ -159,6 +209,10 @@ def register(sub, positive_int) -> None: "--kind", choices=["skills", "tasks", "all"], default="all", help="Restrict the scan to skills, tasks, or both (default: all)", ) + scan_p.add_argument( + "--format", choices=["text", "json"], default="text", + help="text: one line per entry; json: one object with a row per skill and task (default: text)", + ) scan_p.add_argument( "--tasks-subdir", default="evaluation/harbor/tasks", help="Task directory relative to --root (default: evaluation/harbor/tasks, " diff --git a/dokimi/dokimi/external/__init__.py b/dokimi/dokimi/external/__init__.py index 227aa3120..373be7460 100644 --- a/dokimi/dokimi/external/__init__.py +++ b/dokimi/dokimi/external/__init__.py @@ -9,10 +9,14 @@ read_skill_manifest, read_task_manifest, ) +from .validate import Finding, SkillReview, review_skills __all__ = [ "SkillCatalogEntry", "TaskCatalogEntry", "read_skill_manifest", "read_task_manifest", + "Finding", + "SkillReview", + "review_skills", ] diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py new file mode 100644 index 000000000..76a367a82 --- /dev/null +++ b/dokimi/dokimi/external/validate.py @@ -0,0 +1,124 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Review the skills of an external catalog: spec frontmatter, and identical copies. + +Runs after the readers of :mod:`dokimi.external.catalog` and only on skills +that loaded (a load error is already reported as an error by the scan). Checks +follow the Agent Skills spec: + +- ``name`` is the directory name, kebab-case (lowercase letters, digits and + single hyphens) and at most 64 characters. A mismatch or a bad format is an + ``error``; a ``name`` missing from the frontmatter is a ``warn``, because the + loader falls back to the directory name silently. +- ``description`` is at most 1024 characters (``error``). A missing or empty + description is a load error, raised by the loader. + +Identical copies: a ``SKILL.md`` whose bytes are the same as an earlier +skill's is a ``warn`` naming that earlier skill. The model sees only the name and +description, so two such skills compete for the same query even when their bundled +files differ (a plugin copy with an extra ``agents/`` folder is still a mirror). +Mirrors are common (one skill checked in under two plugin directories), and +counting them twice inflates routing and coverage numbers. + +``disable-model-invocation: true`` is not a finding: the model is never offered +that skill, so it is reported as a flag (the same reading as the skill arm's +check in :mod:`dokimi.harnesses.skill_treatment`). +""" + +from __future__ import annotations + +import hashlib +import re +from dataclasses import dataclass, field +from typing import Optional, Sequence + +from dokimi.skills import Skill +from dokimi.utils import parse_frontmatter + +from .catalog import SkillCatalogEntry + +NAME_PATTERN = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*") +NAME_MAX = 64 +DESCRIPTION_MAX = 1024 + + +@dataclass +class Finding: + """One check that did not pass: ``level`` is ``warn`` or ``error``.""" + + level: str + code: str + message: str + + +@dataclass +class SkillReview: + """What :func:`review_skills` found for one loaded skill.""" + + findings: list[Finding] = field(default_factory=list) + disable_model_invocation: bool = False + content_sha256: Optional[str] = None + duplicate_of: Optional[str] = None # the skill directory of the earlier copy, relative to the catalog root + + @property + def state(self) -> str: + """``error`` if any finding is an error, else ``warn`` if any is a warning, else ``ok``.""" + levels = {f.level for f in self.findings} + return "error" if "error" in levels else "warn" if "warn" in levels else "ok" + + +def _frontmatter_findings(skill: Skill) -> tuple[list[Finding], bool]: + """The spec checks on the frontmatter of ``skill``, and its ``disable-model-invocation`` flag.""" + meta, _ = parse_frontmatter(skill.path.read_text(encoding="utf-8")) + dir_name = skill.path.parent.name + findings: list[Finding] = [] + declared = str(meta.get("name") or "").strip() + if not declared: + findings.append(Finding("warn", "name-missing", + f"no name in the frontmatter; the directory name {dir_name!r} is used")) + else: + if declared != dir_name: + findings.append(Finding("error", "name-mismatch", + f"name {declared!r} is not the directory name {dir_name!r}")) + if len(declared) > NAME_MAX or not NAME_PATTERN.fullmatch(declared): + findings.append(Finding("error", "name-format", + f"name {declared!r} is not kebab-case (lowercase letters, digits, " + f"single hyphens; at most {NAME_MAX} characters)")) + if len(skill.description) > DESCRIPTION_MAX: + findings.append(Finding("error", "description-too-long", + f"description is {len(skill.description)} characters; the limit is {DESCRIPTION_MAX}")) + # Read as the skill arm reads it (dokimi.harnesses.skill_treatment.check_skill). + hidden = str(meta.get("disable-model-invocation", "")).strip().lower() in ("1", "true", "yes", "on") + return findings, hidden + + +def _skill_digest(skill: Skill) -> str: + """SHA-256 of the ``SKILL.md`` bytes: the text the model is shown when it chooses a skill.""" + return hashlib.sha256(skill.path.read_bytes()).hexdigest() + + +def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillReview]]: + """One :class:`SkillReview` per entry, in order; None for an entry that did not load. + + The first skill of each identical ``SKILL.md`` is the original; a later copy names it in + ``duplicate_of``. + """ + reviews: list[Optional[SkillReview]] = [] + first_by_digest: dict[str, Skill] = {} + for entry in entries: + if not entry.loaded or entry.skill is None: + reviews.append(None) + continue + skill = entry.skill + findings, hidden = _frontmatter_findings(skill) + review = SkillReview(findings=findings, disable_model_invocation=hidden) + review.content_sha256 = _skill_digest(skill) + original = first_by_digest.setdefault(review.content_sha256, skill) + if original is not skill: + where = original.path.parent.relative_to(entry.source_root).as_posix() + review.duplicate_of = where + review.findings.append(Finding("warn", "duplicate", + f"SKILL.md is identical to {where}; the same skill is counted twice")) + reviews.append(review) + return reviews diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py new file mode 100644 index 000000000..f566b3ae2 --- /dev/null +++ b/dokimi/tests/test_catalog_validate.py @@ -0,0 +1,173 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Spec and mirror review of ``catalog scan`` (``dokimi.external.validate``), and its ``--format json`` output.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from dokimi.cli import build_parser +from dokimi.external import read_skill_manifest, review_skills + + +def _skill(root: Path, rel: str, name: str | None = None, description: str = "Use for things.", + extra: str = "", body: str = "# Body\n") -> Path: + """Write ``//SKILL.md``; ``name`` None leaves the key out of the frontmatter.""" + skill_dir = root / rel + skill_dir.mkdir(parents=True, exist_ok=True) + head = f"name: {name}\n" if name is not None else "" + (skill_dir / "SKILL.md").write_text( + f"---\n{head}description: {description}\n{extra}---\n{body}", encoding="utf-8") + return skill_dir + + +def _scan_json(root: Path, capsys, *, kind: str = "skills") -> tuple[dict, int]: + """Run ``catalog scan --format json``; return the parsed report and the exit code (0 when none raised).""" + args = build_parser().parse_args( + ["catalog", "scan", "--root", str(root), "--kind", kind, "--format", "json"]) + code = 0 + try: + args.func(args) + except SystemExit as exc: + code = exc.code + return json.loads(capsys.readouterr().out), code + + +def _reviews_by_name(root: Path) -> dict: + entries = read_skill_manifest(root) + return {e.name: r for e, r in zip(entries, review_skills(entries), strict=True)} + + +def test_name_that_is_not_the_directory_name_is_an_error(tmp_path): + _skill(tmp_path / "skills", "writing-rules", name="Writing Hookify Rules") + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.state == "error" + codes = {f.code for f in review.findings} + assert {"name-mismatch", "name-format"} <= codes + + +def test_title_case_name_matching_directory_is_still_not_kebab_case(tmp_path): + _skill(tmp_path / "skills", "Writing", name="Writing") + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.state == "error" + assert [f.code for f in review.findings] == ["name-format"] + + +def test_kebab_case_name_matching_directory_is_ok(tmp_path): + _skill(tmp_path / "skills", "linux-perf-2", name="linux-perf-2") + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.state == "ok" + assert review.findings == [] + + +def test_name_missing_from_frontmatter_is_a_warning(tmp_path): + _skill(tmp_path / "skills", "no-name", name=None) + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.state == "warn" + assert [f.code for f in review.findings] == ["name-missing"] + + +def test_description_length_limit_is_1024_characters(tmp_path): + _skill(tmp_path / "skills", "at-limit", name="at-limit", description="d" * 1024) + _skill(tmp_path / "skills", "over-limit", name="over-limit", description="d" * 1025) + + reviews = _reviews_by_name(tmp_path) + assert reviews["at-limit"].state == "ok" + over = reviews["over-limit"] + assert over.state == "error" + assert [f.code for f in over.findings] == ["description-too-long"] + + +def test_identical_skill_md_is_a_warning_naming_the_first_copy(tmp_path): + # A plugin copy with an extra bundled folder is still the same SKILL.md: the model sees the same text. + _skill(tmp_path / "plugins", "x", name="x", description="Use for x.") + copy = _skill(tmp_path / "skills", "x", name="x", description="Use for x.") + (copy / "agents").mkdir() + (copy / "agents" / "openai.yaml").write_text("a: 1\n", encoding="utf-8") + + reviews = review_skills(read_skill_manifest(tmp_path)) + originals = [r for r in reviews if r.duplicate_of is None] + copies = [r for r in reviews if r.duplicate_of is not None] + assert len(originals) == 1 and len(copies) == 1 + assert copies[0].duplicate_of == "plugins/x" + assert copies[0].state == "warn" + assert copies[0].findings[-1].code == "duplicate" + assert copies[0].content_sha256 == originals[0].content_sha256 + + +def test_different_skill_md_with_the_same_name_is_not_a_duplicate(tmp_path): + _skill(tmp_path / "plugins", "x", name="x", description="Use for x.") + _skill(tmp_path / "skills", "x", name="x", description="Use for x, a different text.") + + reviews = review_skills(read_skill_manifest(tmp_path)) + assert all(r.duplicate_of is None for r in reviews) + assert all(r.state == "ok" for r in reviews) + + +def test_disable_model_invocation_is_reported_as_a_flag_not_a_finding(tmp_path): + _skill(tmp_path / "skills", "hidden", name="hidden", + extra="disable-model-invocation: true\n") + _skill(tmp_path / "skills", "shown", name="shown") + + reviews = _reviews_by_name(tmp_path) + assert reviews["hidden"].disable_model_invocation is True + assert reviews["hidden"].state == "ok" + assert reviews["shown"].disable_model_invocation is False + + +def test_json_report_carries_the_findings_and_exits_1_on_an_error(tmp_path, capsys): + _skill(tmp_path / "skills", "claude-api", name="Claude API") + report, code = _scan_json(tmp_path, capsys) + + assert code == 1 + [row] = report["skills"] + assert row["state"] == "error" + assert row["error"] is None + assert {f["code"] for f in row["findings"]} >= {"name-mismatch", "name-format"} + assert row["content_sha256"] and len(row["content_sha256"]) == 64 + + +def test_json_report_on_warnings_only_exits_0(tmp_path, capsys): + _skill(tmp_path / "plugins", "x", name="x") + _skill(tmp_path / "skills", "x", name="x") + _skill(tmp_path / "skills", "hidden", name="hidden", extra="disable-model-invocation: yes\n") + report, code = _scan_json(tmp_path, capsys) + + assert code == 0 + states = {row["path"].split(str(tmp_path) + "/")[1]: row["state"] for row in report["skills"]} + assert states == {"plugins/x": "ok", "skills/x": "warn", "skills/hidden": "ok"} + hidden = next(r for r in report["skills"] if r["name"] == "hidden") + assert hidden["disable_model_invocation"] is True + assert report["tasks"] == [] + + +def test_json_report_keeps_load_errors_as_error_rows(tmp_path, capsys): + (tmp_path / "skills.yaml").write_text( + 'skills:\n - name: ghost\n catalog_path: skills/does-not-exist\n', encoding="utf-8") + report, code = _scan_json(tmp_path, capsys) + + assert code == 1 + [row] = report["skills"] + assert row["state"] == "error" + assert "does-not-exist" in row["error"] + + +def test_text_output_lists_each_finding_under_its_skill(tmp_path, capsys): + _skill(tmp_path / "skills", "claude-api", name="Claude API") + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path), "--kind", "skills"]) + with pytest.raises(SystemExit) as exc_info: + args.func(args) + assert exc_info.value.code == 1 + out = capsys.readouterr().out + assert "# skills (1 declared" in out and "1 error" in out + assert "ERROR: name 'Claude API' is not the directory name" in out + assert " error name-mismatch:" in out + assert " error name-format:" in out From 1868f79f0e9a31655443198f30bcd60acb542768 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Wed, 21 Oct 2026 22:12:54 -0700 Subject: [PATCH 02/11] fix(catalog): spec errors exit 0 without --strict; name broken YAML `catalog scan` exited 1 on a skill spec error. Spec errors now print and exit 0; `--strict` makes them exit 1, as a load error does. Load errors still exit 1 always. A SKILL.md whose frontmatter is not valid YAML is reported as a `frontmatter-parse` error carrying the YAML message. The loader reads that as "no description", so the scan showed only a misleading missing description error before. The scan uses a new parse_frontmatter_strict path; parse_frontmatter is unchanged for other callers. Without a skills.yaml, the scan's discovery path now records a broken skill as an error row instead of stopping at the first one. validate.py is ruff-formatted; catalog.py only in the lines this branch added. Signed-off-by: Nikolay Petrov --- README.md | 2 +- ...50000_napetrov_catalog-scan-spec-review.md | 16 ++- dokimi/docs/external-content.md | 25 ++-- dokimi/dokimi/commands/catalog.py | 48 +++++--- dokimi/dokimi/external/catalog.py | 37 ++++-- dokimi/dokimi/external/validate.py | 110 ++++++++++++++---- dokimi/dokimi/utils.py | 23 +++- dokimi/tests/test_catalog_validate.py | 75 ++++++++++-- 8 files changed, 267 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 05b341d2c..e5ac18a97 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ the scores. `novelty` refuses a self-judge; `arms run` does not check, so name | Step | Command | Cost | |---|---|---| -| 1. Does it load? | `dokimi catalog scan --root --kind skills` | free | +| 1. Does it load? | `dokimi catalog scan --root --kind skills [--strict]` | free | | 2. Is it ever picked? | `dokimi novelty routing --artifact skill: --catalog --queries queries.json [--model …]` | free without `--model` | | 3. What will step 4 cost? | `dokimi novelty analyze --artifact skill: --tier triage --include-resources --estimate-only` | free | | 4. What does it add? | the same command without `--estimate-only`, plus `--model`, `--provider`, `--judge-model` and `--max-usd` | ~100 calls | diff --git a/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md index bc3a863b2..61aee1ae4 100644 --- a/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md +++ b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md @@ -7,14 +7,18 @@ with its first copy), is a `warn`. `disable-model-invocation: true` is printed as a note, since the model is never offered that skill. Each finding is printed under its skill. +- **`catalog scan --strict`** — exits 1 on a skill spec error as well as on a skill that + does not load. Without it a spec error is printed and the exit code stays 0. - **`catalog scan --format json`** — one object with a row per skill (`state` is `ok`, `warn` or `error`, `findings`, `content_sha256`, `duplicate_of`) and per task (`state` is `ok`, `skipped` or `error`), for a CI gate or a script. -### Changed +### Fixed -- **`catalog scan` exits 1 on a skill spec error** (a name that is not the directory - name or not kebab-case, a description over 1024 characters), as it already did for a - skill that does not load. Warnings (mirrors, a missing `name`) do not change the exit - code. A catalog that declares such a skill now fails the scan; fix the frontmatter - or drop the skill from the scan. +- **`catalog scan` names broken YAML frontmatter** — a `SKILL.md` whose frontmatter is not + valid YAML (a bare colon in `description:`, say) is reported as a `frontmatter-parse` + error with the YAML message, and exits 1. The scan showed only "missing a description" + before. `parse_frontmatter` is unchanged for other callers. +- **`catalog scan` without a `skills.yaml` no longer stops at the first broken skill** — each + `SKILL.md` under the root is an entry, so a skill that does not load is an error row and + the rest are still reviewed. diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index efc5aac9b..d12aaf840 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -109,21 +109,30 @@ Read that output as three outcomes, not two: drive (`answer-quality` entries are question-set material, not Docker tasks). - `ERROR` — an entry that should have worked and did not. -Only errors change the exit code, so `catalog scan` exiting 0 means "everything -declared as runnable is runnable". Note the asymmetry above: a `planned` *task* +Only load errors change the exit code, so `catalog scan` exiting 0 means +"everything declared as runnable is runnable". A spec error (below) changes it +only under `--strict`. Note the asymmetry above: a `planned` *task* with no directory is skipped, while a `planned` *skill* with no directory is an error — the skill reader has no notion of a not-yet-written entry. Either drop such a skill from `skills.yaml` or expect the non-zero exit. A loaded skill is also checked against the Agent Skills spec. Its `name` must be its directory name and kebab-case (at most 64 characters), and its `description` -at most 1024 characters: a violation is an `error` and exits 1, as a load error -does. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to an -earlier skill's, is a `warn` that names the first copy; warnings do not change +at most 1024 characters: a violation is an `error`, printed under the skill. It +exits 1 only with `--strict`; without it, the scan reports the error and still +exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to +an earlier skill's, is a `warn` that names the first copy; warnings never change the exit code. `disable-model-invocation: true` is printed as a `note`, because -the model is never offered that skill. Each finding is printed under its skill. -`--format json` prints the same rows as one object (`state`, `findings`, -`content_sha256`, `duplicate_of`) for a script or a CI gate. +the model is never offered that skill. + +Frontmatter that is not valid YAML (a bare colon in `description:`, say) stops +the skill from loading, since the loader reads it as having no description. The +scan reports that as a `frontmatter-parse` error carrying the YAML message, and +it exits 1 with or without `--strict`, as a load error does. + +`--strict` makes `catalog scan` a gate on the spec as well as on loading. The +text output and `--format json` (one object: `state`, `findings`, +`content_sha256`, `duplicate_of`, for a script or a CI gate) are the same either way. Restrict the scan with `--kind`: diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index 270730332..987444f22 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -20,9 +20,10 @@ def _skill_row(skill, review) -> dict: """One skill of the scan report. ``state`` is ``error`` for a load error or an error finding, - ``warn`` for a warning finding only, else ``ok``.""" + ``warn`` for a warning finding only, else ``ok``. A skill that did not load has a review only + when its frontmatter is broken YAML, and that review's finding is its only finding.""" if not skill.loaded: - state, findings = "error", [] + state, findings = "error", (review.findings if review else []) else: state, findings = review.state, review.findings return { @@ -61,11 +62,14 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: row = _skill_row(skill, review) counts[row["state"]] += 1 report["skills"].append(row) - if row["state"] == "error": + # A load error always fails; a spec error fails only with --strict. + if not skill.loaded or (args.strict and row["state"] == "error"): any_failure = True if not as_json: - print(f"# skills ({len(skills)} declared under {root}; " - f"{counts['ok']} ok, {counts['warn']} warn, {counts['error']} error)") + print( + f"# skills ({len(skills)} declared under {root}; " + f"{counts['ok']} ok, {counts['warn']} warn, {counts['error']} error)" + ) for row in report["skills"]: if row["error"] is not None: state = f"ERROR: {row['error']}" @@ -74,11 +78,15 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: else: worst = next(f for f in row["findings"] if f["level"] == row["state"]) state = f"{'ERROR' if row['state'] == 'error' else 'warn'}: {worst['message']}" - print(f"{row['name']}\t{row['manifest_status'] or '-'}\t{row['path'] or '-'}\t{state}") + print( + f"{row['name']}\t{row['manifest_status'] or '-'}\t{row['path'] or '-'}\t{state}" + ) for f in row["findings"]: print(f" {f['level']} {f['code']}: {f['message']}") if row["disable_model_invocation"]: - print(" note disable-model-invocation: true (the model is never offered this skill)") + print( + " note disable-model-invocation: true (the model is never offered this skill)" + ) print() if args.kind in ("tasks", "all"): @@ -96,12 +104,16 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: else: state = f"ERROR: {task.reason}" any_failure = True - report["tasks"].append({ - "name": task.name, "manifest_status": task.status, "track": task.track, - "path": str(task.path) if task.path else None, - "state": "ok" if task.loaded else "skipped" if task.skipped else "error", - "reason": task.reason, - }) + report["tasks"].append( + { + "name": task.name, + "manifest_status": task.status, + "track": task.track, + "path": str(task.path) if task.path else None, + "state": "ok" if task.loaded else "skipped" if task.skipped else "error", + "reason": task.reason, + } + ) if not as_json: print(f"{task.name}\t{task.status or '-'}\t{task.track or '-'}\t{state}") @@ -210,7 +222,15 @@ def register(sub, positive_int) -> None: help="Restrict the scan to skills, tasks, or both (default: all)", ) scan_p.add_argument( - "--format", choices=["text", "json"], default="text", + "--strict", + action="store_true", + help="Exit 1 on a skill spec error (name not the directory name or not kebab-case, " + "description over 1024 characters) as well as on a skill that does not load", + ) + scan_p.add_argument( + "--format", + choices=["text", "json"], + default="text", help="text: one line per entry; json: one object with a row per skill and task (default: text)", ) scan_p.add_argument( diff --git a/dokimi/dokimi/external/catalog.py b/dokimi/dokimi/external/catalog.py index b8b221480..7b0d5336b 100644 --- a/dokimi/dokimi/external/catalog.py +++ b/dokimi/dokimi/external/catalog.py @@ -29,7 +29,7 @@ from typing import Optional from dokimi.harnesses.tasks import TaskSpec, discover_tasks, is_task_dir, load_task -from dokimi.skills import Skill, discover_skills, load_skill +from dokimi.skills import Skill, load_skill DEFAULT_TASKS_SUBDIR = "evaluation/harbor/tasks" DEFAULT_SUITES_MANIFEST = "evaluation/harbor/suites.json" @@ -99,16 +99,33 @@ def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: root = Path(root) manifest_path = root / "skills.yaml" if not manifest_path.is_file(): - return [ - SkillCatalogEntry( - name=skill.name, - status=None, - path=skill.path.parent, - source_root=root, - skill=skill, + # One entry per SKILL.md, as the manifest path gives: a skill that does not + # load is an error row, so one broken skill does not abort the whole scan. + found: list[SkillCatalogEntry] = [] + for skill_file in sorted(root.rglob("SKILL.md")): + try: + skill = load_skill(skill_file) + except (FileNotFoundError, ValueError) as exc: + found.append( + SkillCatalogEntry( + name=skill_file.parent.name, + status=None, + path=skill_file.parent, + source_root=root, + error=str(exc), + ) + ) + continue + found.append( + SkillCatalogEntry( + name=skill.name, + status=None, + path=skill.path.parent, + source_root=root, + skill=skill, + ) ) - for skill in discover_skills(root) - ] + return found import yaml diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 76a367a82..002c611ba 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -3,8 +3,11 @@ """Review the skills of an external catalog: spec frontmatter, and identical copies. -Runs after the readers of :mod:`dokimi.external.catalog` and only on skills -that loaded (a load error is already reported as an error by the scan). Checks +Runs after the readers of :mod:`dokimi.external.catalog`. A skill that loaded +gets every check below. A skill that did not load gets one check only: broken +YAML in its frontmatter, reported as ``frontmatter-parse`` with the YAML error, +because the loader reads broken YAML as "no description" and its load error +would otherwise hide the cause. Other load errors stay the scan's own. Checks follow the Agent Skills spec: - ``name`` is the directory name, kebab-case (lowercase letters, digits and @@ -13,6 +16,7 @@ loader falls back to the directory name silently. - ``description`` is at most 1024 characters (``error``). A missing or empty description is a load error, raised by the loader. +- The frontmatter is valid YAML (``frontmatter-parse``, ``error``). Identical copies: a ``SKILL.md`` whose bytes are the same as an earlier skill's is a ``warn`` naming that earlier skill. The model sees only the name and @@ -33,8 +37,10 @@ from dataclasses import dataclass, field from typing import Optional, Sequence +import yaml + from dokimi.skills import Skill -from dokimi.utils import parse_frontmatter +from dokimi.utils import parse_frontmatter_strict from .catalog import SkillCatalogEntry @@ -59,7 +65,9 @@ class SkillReview: findings: list[Finding] = field(default_factory=list) disable_model_invocation: bool = False content_sha256: Optional[str] = None - duplicate_of: Optional[str] = None # the skill directory of the earlier copy, relative to the catalog root + duplicate_of: Optional[str] = ( + None # the skill directory of the earlier copy, relative to the catalog root + ) @property def state(self) -> str: @@ -68,28 +76,67 @@ def state(self) -> str: return "error" if "error" in levels else "warn" if "warn" in levels else "ok" +def _strict_meta(text: str) -> tuple[dict, Optional[Finding]]: + """The frontmatter of ``text``, or a ``frontmatter-parse`` error carrying the YAML message.""" + try: + meta, _ = parse_frontmatter_strict(text) + except yaml.YAMLError as exc: + message = " ".join(str(exc).split()) # the YAML message is several lines; keep one + return {}, Finding( + "error", "frontmatter-parse", f"frontmatter is not valid YAML: {message}" + ) + return meta, None + + def _frontmatter_findings(skill: Skill) -> tuple[list[Finding], bool]: """The spec checks on the frontmatter of ``skill``, and its ``disable-model-invocation`` flag.""" - meta, _ = parse_frontmatter(skill.path.read_text(encoding="utf-8")) + meta, parse_error = _strict_meta(skill.path.read_text(encoding="utf-8")) + if parse_error is not None: + return [parse_error], False dir_name = skill.path.parent.name findings: list[Finding] = [] declared = str(meta.get("name") or "").strip() if not declared: - findings.append(Finding("warn", "name-missing", - f"no name in the frontmatter; the directory name {dir_name!r} is used")) + findings.append( + Finding( + "warn", + "name-missing", + f"no name in the frontmatter; the directory name {dir_name!r} is used", + ) + ) else: if declared != dir_name: - findings.append(Finding("error", "name-mismatch", - f"name {declared!r} is not the directory name {dir_name!r}")) + findings.append( + Finding( + "error", + "name-mismatch", + f"name {declared!r} is not the directory name {dir_name!r}", + ) + ) if len(declared) > NAME_MAX or not NAME_PATTERN.fullmatch(declared): - findings.append(Finding("error", "name-format", - f"name {declared!r} is not kebab-case (lowercase letters, digits, " - f"single hyphens; at most {NAME_MAX} characters)")) + findings.append( + Finding( + "error", + "name-format", + f"name {declared!r} is not kebab-case (lowercase letters, digits, " + f"single hyphens; at most {NAME_MAX} characters)", + ) + ) if len(skill.description) > DESCRIPTION_MAX: - findings.append(Finding("error", "description-too-long", - f"description is {len(skill.description)} characters; the limit is {DESCRIPTION_MAX}")) + findings.append( + Finding( + "error", + "description-too-long", + f"description is {len(skill.description)} characters; the limit is {DESCRIPTION_MAX}", + ) + ) # Read as the skill arm reads it (dokimi.harnesses.skill_treatment.check_skill). - hidden = str(meta.get("disable-model-invocation", "")).strip().lower() in ("1", "true", "yes", "on") + hidden = str(meta.get("disable-model-invocation", "")).strip().lower() in ( + "1", + "true", + "yes", + "on", + ) return findings, hidden @@ -98,17 +145,33 @@ def _skill_digest(skill: Skill) -> str: return hashlib.sha256(skill.path.read_bytes()).hexdigest() +def _unloaded_review(entry: SkillCatalogEntry) -> Optional[SkillReview]: + """A review for an entry that did not load, only when its ``SKILL.md`` has broken YAML; else None.""" + if entry.path is None: + return None + candidate = entry.path / "SKILL.md" if entry.path.is_dir() else entry.path + if candidate.name != "SKILL.md" or not candidate.is_file(): + return None + try: + text = candidate.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None # the loader already reported why the skill did not load + _, parse_error = _strict_meta(text) + return None if parse_error is None else SkillReview(findings=[parse_error]) + + def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillReview]]: - """One :class:`SkillReview` per entry, in order; None for an entry that did not load. + """One :class:`SkillReview` per entry, in order. - The first skill of each identical ``SKILL.md`` is the original; a later copy names it in - ``duplicate_of``. + None for an entry that did not load, unless its frontmatter is broken YAML: then its + review carries the ``frontmatter-parse`` error. The first skill of each identical + ``SKILL.md`` is the original; a later copy names it in ``duplicate_of``. """ reviews: list[Optional[SkillReview]] = [] first_by_digest: dict[str, Skill] = {} for entry in entries: if not entry.loaded or entry.skill is None: - reviews.append(None) + reviews.append(_unloaded_review(entry)) continue skill = entry.skill findings, hidden = _frontmatter_findings(skill) @@ -118,7 +181,12 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe if original is not skill: where = original.path.parent.relative_to(entry.source_root).as_posix() review.duplicate_of = where - review.findings.append(Finding("warn", "duplicate", - f"SKILL.md is identical to {where}; the same skill is counted twice")) + review.findings.append( + Finding( + "warn", + "duplicate", + f"SKILL.md is identical to {where}; the same skill is counted twice", + ) + ) reviews.append(review) return reviews diff --git a/dokimi/dokimi/utils.py b/dokimi/dokimi/utils.py index 939771acd..291701ed4 100644 --- a/dokimi/dokimi/utils.py +++ b/dokimi/dokimi/utils.py @@ -12,7 +12,9 @@ def parse_frontmatter(text: str) -> "Tuple[Dict[str, Any], str]": Supports a leading YAML frontmatter block delimited by ``---`` lines, the same convention used by Claude Code skill files and most static-site - generators. When no frontmatter is present, returns ``({}, text)``. + generators. When no frontmatter is present, returns ``({}, text)``. Frontmatter + that is not valid YAML also returns ``{}``; use :func:`parse_frontmatter_strict` + to see that error. Args: text: Raw file contents. @@ -20,6 +22,23 @@ def parse_frontmatter(text: str) -> "Tuple[Dict[str, Any], str]": Returns: Tuple of (metadata dict, body string with frontmatter stripped). """ + return _split_frontmatter(text, strict=False) + + +def parse_frontmatter_strict(text: str) -> "Tuple[Dict[str, Any], str]": + """Like :func:`parse_frontmatter`, but frontmatter that is not valid YAML raises. + + For a validator that must name the YAML error rather than read the document + as having no metadata. The lenient :func:`parse_frontmatter` is unchanged for + every other caller. + + Raises: + yaml.YAMLError: The frontmatter block is not valid YAML. + """ + return _split_frontmatter(text, strict=True) + + +def _split_frontmatter(text: str, *, strict: bool) -> "Tuple[Dict[str, Any], str]": import yaml if not text: @@ -40,6 +59,8 @@ def parse_frontmatter(text: str) -> "Tuple[Dict[str, Any], str]": try: meta = yaml.safe_load(raw_meta) or {} except yaml.YAMLError: + if strict: + raise meta = {} if not isinstance(meta, dict): meta = {} diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index f566b3ae2..a27fdf398 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -9,9 +9,11 @@ from pathlib import Path import pytest +import yaml from dokimi.cli import build_parser from dokimi.external import read_skill_manifest, review_skills +from dokimi.utils import parse_frontmatter, parse_frontmatter_strict def _skill(root: Path, rel: str, name: str | None = None, description: str = "Use for things.", @@ -25,10 +27,12 @@ def _skill(root: Path, rel: str, name: str | None = None, description: str = "Us return skill_dir -def _scan_json(root: Path, capsys, *, kind: str = "skills") -> tuple[dict, int]: +def _scan_json( + root: Path, capsys, *, kind: str = "skills", strict: bool = False +) -> tuple[dict, int]: """Run ``catalog scan --format json``; return the parsed report and the exit code (0 when none raised).""" - args = build_parser().parse_args( - ["catalog", "scan", "--root", str(root), "--kind", kind, "--format", "json"]) + argv = ["catalog", "scan", "--root", str(root), "--kind", kind, "--format", "json"] + args = build_parser().parse_args(argv + (["--strict"] if strict else [])) code = 0 try: args.func(args) @@ -123,11 +127,13 @@ def test_disable_model_invocation_is_reported_as_a_flag_not_a_finding(tmp_path): assert reviews["shown"].disable_model_invocation is False -def test_json_report_carries_the_findings_and_exits_1_on_an_error(tmp_path, capsys): +def test_json_report_carries_the_findings_and_exits_0_on_a_spec_error_without_strict( + tmp_path, capsys +): _skill(tmp_path / "skills", "claude-api", name="Claude API") report, code = _scan_json(tmp_path, capsys) - assert code == 1 + assert code == 0 [row] = report["skills"] assert row["state"] == "error" assert row["error"] is None @@ -135,6 +141,23 @@ def test_json_report_carries_the_findings_and_exits_1_on_an_error(tmp_path, caps assert row["content_sha256"] and len(row["content_sha256"]) == 64 +def test_strict_makes_a_spec_error_exit_1(tmp_path, capsys): + _skill(tmp_path / "skills", "claude-api", name="Claude API") + report, code = _scan_json(tmp_path, capsys, strict=True) + + assert code == 1 + assert report["skills"][0]["state"] == "error" + + +def test_strict_does_not_fail_on_warnings(tmp_path, capsys): + _skill(tmp_path / "plugins", "x", name="x") + _skill(tmp_path / "skills", "x", name="x") + _skill(tmp_path / "skills", "no-name", name=None) + _, code = _scan_json(tmp_path, capsys, strict=True) + + assert code == 0 + + def test_json_report_on_warnings_only_exits_0(tmp_path, capsys): _skill(tmp_path / "plugins", "x", name="x") _skill(tmp_path / "skills", "x", name="x") @@ -163,11 +186,47 @@ def test_json_report_keeps_load_errors_as_error_rows(tmp_path, capsys): def test_text_output_lists_each_finding_under_its_skill(tmp_path, capsys): _skill(tmp_path / "skills", "claude-api", name="Claude API") args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path), "--kind", "skills"]) - with pytest.raises(SystemExit) as exc_info: - args.func(args) - assert exc_info.value.code == 1 + args.func(args) # no SystemExit: a spec error is printed, and only --strict makes it fail out = capsys.readouterr().out assert "# skills (1 declared" in out and "1 error" in out assert "ERROR: name 'Claude API' is not the directory name" in out assert " error name-mismatch:" in out assert " error name-format:" in out + + +BARE_COLON = "Use when: the user asks for a Claude API call" + + +def test_frontmatter_with_a_bare_colon_is_a_frontmatter_parse_error(tmp_path): + # ``description: Use when: ...`` is not valid YAML; the lenient parser reads it as no metadata. + _skill(tmp_path / "skills", "claude-api", name="claude-api", description=BARE_COLON) + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.state == "error" + [finding] = review.findings + assert finding.code == "frontmatter-parse" + assert finding.level == "error" + assert finding.message.startswith( + "frontmatter is not valid YAML: mapping values are not allowed" + ) + assert "\n" not in finding.message + + +def test_broken_yaml_is_reported_in_place_of_the_missing_description_error(tmp_path, capsys): + _skill(tmp_path / "skills", "claude-api", name="claude-api", description=BARE_COLON) + report, code = _scan_json(tmp_path, capsys) + + assert code == 1 # a skill that does not load exits 1 with or without --strict + [row] = report["skills"] + assert row["state"] == "error" + assert "missing a 'description'" in row["error"] + assert [f["code"] for f in row["findings"]] == ["frontmatter-parse"] + assert row["content_sha256"] is None and row["duplicate_of"] is None + + +def test_strict_parse_raises_and_the_lenient_parse_still_returns_no_metadata(): + text = f"---\nname: claude-api\ndescription: {BARE_COLON}\n---\nbody\n" + + assert parse_frontmatter(text) == ({}, "body") + with pytest.raises(yaml.YAMLError): + parse_frontmatter_strict(text) From 7e1d17e811c01beda9d31ec9931f377a536d3bdf Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Thu, 22 Oct 2026 12:34:04 +0000 Subject: [PATCH 03/11] Catch OSError when loading a skill so unreadable files become error rows Signed-off-by: Anton Volkov --- dokimi/dokimi/external/catalog.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dokimi/dokimi/external/catalog.py b/dokimi/dokimi/external/catalog.py index 7b0d5336b..2c6b6098b 100644 --- a/dokimi/dokimi/external/catalog.py +++ b/dokimi/dokimi/external/catalog.py @@ -105,7 +105,7 @@ def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: for skill_file in sorted(root.rglob("SKILL.md")): try: skill = load_skill(skill_file) - except (FileNotFoundError, ValueError) as exc: + except (OSError, ValueError) as exc: found.append( SkillCatalogEntry( name=skill_file.parent.name, @@ -201,7 +201,7 @@ def failed(error: str, path: Optional[Path] = None) -> SkillCatalogEntry: return failed(f"{source} escapes root: {catalog_path!r}") try: skill = load_skill(path) - except (FileNotFoundError, ValueError) as exc: + except (OSError, ValueError) as exc: return failed(str(exc), path) # The directory is inside the root; a SKILL.md symlinked out of it is not. if not _within(skill.path, root): From 1ed1cfb75ce69803008c18cec17f02697bb0f8dd Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Thu, 22 Oct 2026 12:34:35 +0000 Subject: [PATCH 04/11] Update scan example header and explain the broken-YAML row Signed-off-by: Anton Volkov --- dokimi/docs/external-content.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index d12aaf840..e4aa9c8d9 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -89,7 +89,7 @@ to pay to run is half-declared. ```console $ python cli.py catalog scan --root C:/Users/Public/content-repo -# skills (2 declared under C:\Users\Public\content-repo) +# skills (2 declared under C:\Users\Public\content-repo; 1 ok, 0 warn, 1 error) demo-skill published C:\Users\Public\content-repo\skills\demo-skill ok not-written-yet planned C:\Users\Public\content-repo\skills\not-written-yet ERROR: Skill path does not exist: C:\Users\Public\content-repo\skills\not-written-yet @@ -128,7 +128,10 @@ the model is never offered that skill. Frontmatter that is not valid YAML (a bare colon in `description:`, say) stops the skill from loading, since the loader reads it as having no description. The scan reports that as a `frontmatter-parse` error carrying the YAML message, and -it exits 1 with or without `--strict`, as a load error does. +it exits 1 with or without `--strict`, as a load error does. The YAML message is +on the indented `frontmatter-parse` line under the skill (and in its `findings` +in JSON); the row's main line and its JSON `error` field still say the skill is +missing a `description`. `--strict` makes `catalog scan` a gate on the spec as well as on loading. The text output and `--format json` (one object: `state`, `findings`, From 67b683e2244e90df16584c962ff2a836ed70519c Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Thu, 22 Oct 2026 08:06:21 -0700 Subject: [PATCH 05/11] fix(catalog): stringify manifest status in JSON; skip name-mismatch for a root SKILL.md An unquoted YAML date as status crashed catalog scan --format json. A SKILL.md directly in the scan root was compared with the root directory name and failed --strict. Adds the review's mutation-gap tests. Signed-off-by: Nikolay Petrov --- dokimi/dokimi/commands/catalog.py | 4 +- dokimi/dokimi/external/validate.py | 17 ++-- dokimi/tests/test_catalog_validate.py | 114 ++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 7 deletions(-) diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index 987444f22..54195f85a 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -28,7 +28,7 @@ def _skill_row(skill, review) -> dict: state, findings = review.state, review.findings return { "name": skill.name, - "manifest_status": skill.status, + "manifest_status": None if skill.status is None else str(skill.status), "path": str(skill.path) if skill.path else None, "state": state, "error": skill.error, @@ -107,7 +107,7 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: report["tasks"].append( { "name": task.name, - "manifest_status": task.status, + "manifest_status": None if task.status is None else str(task.status), "track": task.track, "path": str(task.path) if task.path else None, "state": "ok" if task.loaded else "skipped" if task.skipped else "error", diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 002c611ba..c9e068c56 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -88,8 +88,12 @@ def _strict_meta(text: str) -> tuple[dict, Optional[Finding]]: return meta, None -def _frontmatter_findings(skill: Skill) -> tuple[list[Finding], bool]: - """The spec checks on the frontmatter of ``skill``, and its ``disable-model-invocation`` flag.""" +def _frontmatter_findings(skill: Skill, *, at_root: bool = False) -> tuple[list[Finding], bool]: + """The spec checks on the frontmatter of ``skill``, and its ``disable-model-invocation`` flag. + + ``at_root``: the ``SKILL.md`` sits directly in the scanned root, so its directory name is the + name of a checkout, which says nothing about the skill; the name/directory comparison is skipped. + """ meta, parse_error = _strict_meta(skill.path.read_text(encoding="utf-8")) if parse_error is not None: return [parse_error], False @@ -101,11 +105,12 @@ def _frontmatter_findings(skill: Skill) -> tuple[list[Finding], bool]: Finding( "warn", "name-missing", - f"no name in the frontmatter; the directory name {dir_name!r} is used", + "no name in the frontmatter" + + ("" if at_root else f"; the directory name {dir_name!r} is used"), ) ) else: - if declared != dir_name: + if declared != dir_name and not at_root: findings.append( Finding( "error", @@ -174,7 +179,9 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe reviews.append(_unloaded_review(entry)) continue skill = entry.skill - findings, hidden = _frontmatter_findings(skill) + findings, hidden = _frontmatter_findings( + skill, at_root=skill.path.parent == entry.source_root + ) review = SkillReview(findings=findings, disable_model_invocation=hidden) review.content_sha256 = _skill_digest(skill) original = first_by_digest.setdefault(review.content_sha256, skill) diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index a27fdf398..785a7453f 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -230,3 +230,117 @@ def test_strict_parse_raises_and_the_lenient_parse_still_returns_no_metadata(): assert parse_frontmatter(text) == ({}, "body") with pytest.raises(yaml.YAMLError): parse_frontmatter_strict(text) + + +# --- review follow-up: edges of the name rules, duplicates, state, JSON rows ------------------------- + + +@pytest.mark.parametrize("name", ["a" * 64, "a1-2b"]) +def test_valid_names_matching_the_directory_have_no_finding(tmp_path, name): + _skill(tmp_path / "skills", name, name=name) + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.findings == [] + + +@pytest.mark.parametrize("name", ["a" * 65, "a--b", "a-", "-a", "a_b", "a b"]) +def test_malformed_names_matching_the_directory_only_fail_name_format(tmp_path, name): + _skill(tmp_path / "skills", name, name=f"'{name}'") + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert [f.code for f in review.findings] == ["name-format"] + + +def test_same_frontmatter_with_a_different_body_is_not_a_duplicate(tmp_path): + _skill(tmp_path / "plugins", "x", name="x", body="# One\n") + _skill(tmp_path / "skills", "x", name="x", body="# Two\n") + + assert all(r.duplicate_of is None for r in review_skills(read_skill_manifest(tmp_path))) + + +def test_a_trailing_newline_difference_is_not_a_duplicate(tmp_path): + _skill(tmp_path / "plugins", "x", name="x", body="# Body\n") + _skill(tmp_path / "skills", "x", name="x", body="# Body\n\n") + + assert all(r.duplicate_of is None for r in review_skills(read_skill_manifest(tmp_path))) + + +def test_three_identical_copies_all_name_the_first(tmp_path): + for parent in ("a-plugins", "b-plugins", "c-plugins"): + _skill(tmp_path / parent, "x", name="x") + + reviews = review_skills(read_skill_manifest(tmp_path)) + assert reviews[0].duplicate_of is None + assert [r.duplicate_of for r in reviews[1:]] == ["a-plugins/x", "a-plugins/x"] + + +def test_an_error_outranks_a_warning_in_the_state_and_fails_strict(tmp_path, capsys): + _skill(tmp_path / "plugins", "Bad", name="Bad") + _skill(tmp_path / "skills", "Bad", name="Bad") + + reviews = review_skills(read_skill_manifest(tmp_path)) + mirror = reviews[1] + assert {f.level for f in mirror.findings} == {"warn", "error"} + assert mirror.state == "error" + _, code = _scan_json(tmp_path, capsys, strict=True) + assert code == 1 + + +def test_disable_model_invocation_false_does_not_hide_the_skill(tmp_path): + _skill(tmp_path / "skills", "shown", name="shown", extra="disable-model-invocation: false\n") + + [review] = review_skills(read_skill_manifest(tmp_path)) + assert review.disable_model_invocation is False + + +def test_json_report_with_an_unquoted_date_status_does_not_crash(tmp_path, capsys): + _skill(tmp_path / "skills", "a", name="a") + (tmp_path / "skills.yaml").write_text( + "skills:\n - name: a\n status: 2026-01-01\n catalog_path: skills/a\n", + encoding="utf-8", + ) + report, code = _scan_json(tmp_path, capsys) + + assert code == 0 + assert report["skills"][0]["manifest_status"] == "2026-01-01" + + +def test_skill_at_the_scan_root_is_not_compared_with_the_root_directory_name(tmp_path, capsys): + root = tmp_path / "checkout" + root.mkdir() + (root / "SKILL.md").write_text( + "---\nname: z\ndescription: Use for z.\n---\n# z\n", encoding="utf-8" + ) + report, code = _scan_json(root, capsys, strict=True) + + assert code == 0 + assert report["skills"][0]["state"] == "ok" + assert report["skills"][0]["findings"] == [] + + +def test_json_tasks_array_carries_loaded_and_skipped_rows(tmp_path, capsys): + _skill(tmp_path / "skills", "s", name="s") + task = tmp_path / "evaluation" / "harbor" / "tasks" / "real" + for sub in ("environment", "tests", "solution"): + (task / sub).mkdir(parents=True) + (task / "instruction.md").write_text("Do it.", encoding="utf-8") + (task / "task.toml").write_text( + '[metadata]\ndifficulty = "easy"\n\n[agent]\ntimeout_sec = 60\n\n' + "[verifier]\ntimeout_sec = 10\n\n[environment]\n", + encoding="utf-8", + ) + (tmp_path / "skills.yaml").write_text( + 'skills:\n - name: s\n catalog_path: skills/s\n', encoding="utf-8") + (tmp_path / "evaluation" / "harbor" / "suites.json").write_text( + json.dumps({"schema_version": "1.0", "suites": [{"skill": "s", "tasks": [ + {"name": "real", "status": "implemented", "track": "executable"}, + {"name": "later", "status": "planned", "track": "executable"}]}]}), + encoding="utf-8", + ) + report, code = _scan_json(tmp_path, capsys, kind="all") + + assert code == 0 + rows = {r["name"]: r for r in report["tasks"]} + assert rows["real"]["state"] == "ok" and rows["real"]["track"] == "executable" + assert rows["real"]["manifest_status"] == "implemented" + assert rows["later"]["state"] == "skipped" and rows["later"]["reason"] From 9ec812d04118ca38c5ad5f230e0969ec754c5aa6 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Fri, 23 Oct 2026 10:42:30 +0000 Subject: [PATCH 06/11] Note that a root SKILL.md skips the directory-name check in docs Signed-off-by: Anton Volkov --- dokimi/docs/external-content.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index e4aa9c8d9..b49aa3394 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -117,7 +117,7 @@ error — the skill reader has no notion of a not-yet-written entry. Either drop such a skill from `skills.yaml` or expect the non-zero exit. A loaded skill is also checked against the Agent Skills spec. Its `name` must be -its directory name and kebab-case (at most 64 characters), and its `description` +its directory name and kebab-case (at most 64 characters; a `SKILL.md` directly in the scanned root is not compared with the root directory's name), and its `description` at most 1024 characters: a violation is an `error`, printed under the skill. It exits 1 only with `--strict`; without it, the scan reports the error and still exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to From a3bb3c41d11870be75ae56a3fd85e4aa1aa4f137 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Fri, 23 Oct 2026 12:45:34 -0700 Subject: [PATCH 07/11] refactor(catalog): split cmd_catalog_scan and read_skill_manifest under the complexity ratchet Signed-off-by: Nikolay Petrov --- dokimi/dokimi/commands/catalog.py | 148 +++++++++++++++------------- dokimi/dokimi/external/catalog.py | 71 +++++++------ dokimi/scripts/growth_baseline.json | 1 - 3 files changed, 122 insertions(+), 98 deletions(-) diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index 27a982ce4..67e36f258 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -39,10 +39,86 @@ def _skill_row(skill, review) -> dict: } -def cmd_catalog_scan(args: argparse.Namespace) -> None: - from dokimi.external import read_skill_manifest, read_task_manifest +def _print_skill_row(row: dict) -> None: + if row["error"] is not None: + state = f"ERROR: {row['error']}" + elif row["state"] == "ok": + state = "ok" + else: + worst = next(f for f in row["findings"] if f["level"] == row["state"]) + state = f"{'ERROR' if row['state'] == 'error' else 'warn'}: {worst['message']}" + print(f"{row['name']}\t{row['manifest_status'] or '-'}\t{row['path'] or '-'}\t{state}") + for f in row["findings"]: + print(f" {f['level']} {f['code']}: {f['message']}") + if row["disable_model_invocation"]: + print(" note disable-model-invocation: true (the model is never offered this skill)") + + +def _scan_skills(args: argparse.Namespace, root: Path, report: dict, as_json: bool) -> bool: + """Review the skills under *root* into ``report``; True when the scan must exit 1.""" + from dokimi.external import read_skill_manifest from dokimi.external.validate import review_skills + try: + skills = read_skill_manifest(root) + except ValueError as exc: + raise SystemExit(f"skills.yaml: {exc}") from exc + reviews = review_skills(skills) + counts = {"ok": 0, "warn": 0, "error": 0} + failed = False + for skill, review in zip(skills, reviews, strict=True): + row = _skill_row(skill, review) + counts[row["state"]] += 1 + report["skills"].append(row) + # A load error always fails; a spec error fails only with --strict. + if not skill.loaded or (args.strict and row["state"] == "error"): + failed = True + if not as_json: + print( + f"# skills ({len(skills)} declared under {root}; " + f"{counts['ok']} ok, {counts['warn']} warn, {counts['error']} error)" + ) + for row in report["skills"]: + _print_skill_row(row) + print() + return failed + + +def _task_row(task) -> dict: + return { + "name": task.name, + "manifest_status": None if task.status is None else str(task.status), + "track": task.track, + "path": str(task.path) if task.path else None, + "state": "ok" if task.loaded else "skipped" if task.skipped else "error", + "reason": task.reason, + } + + +def _scan_tasks(args: argparse.Namespace, root: Path, report: dict, as_json: bool) -> bool: + """List the tasks under *root* into ``report``; True when the scan must exit 1.""" + from dokimi.external import read_task_manifest + + try: + tasks = read_task_manifest(root, tasks_subdir=args.tasks_subdir) + except ValueError as exc: + raise SystemExit(f"suites.json: {exc}") from exc + if not as_json: + print(f"# tasks ({len(tasks)} declared under {root})") + failed = False + for task in tasks: + row = _task_row(task) + report["tasks"].append(row) + failed = failed or row["state"] == "error" + if not as_json: + state = {"ok": "ok", "skipped": f"skipped: {task.reason}"}.get( + row["state"], f"ERROR: {task.reason}" + ) + print(f"{task.name}\t{task.status or '-'}\t{task.track or '-'}\t{state}") + return failed + + +def cmd_catalog_scan(args: argparse.Namespace) -> None: root = Path(args.root) if not root.is_dir(): raise SystemExit(f"--root is not a directory: {root}") @@ -50,76 +126,12 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: as_json = args.format == "json" report: dict = {"root": str(root), "skills": [], "tasks": []} any_failure = False - if args.kind in ("skills", "all"): - try: - skills = read_skill_manifest(root) - except ValueError as exc: - raise SystemExit(f"skills.yaml: {exc}") from exc - reviews = review_skills(skills) - counts = {"ok": 0, "warn": 0, "error": 0} - for skill, review in zip(skills, reviews, strict=True): - row = _skill_row(skill, review) - counts[row["state"]] += 1 - report["skills"].append(row) - # A load error always fails; a spec error fails only with --strict. - if not skill.loaded or (args.strict and row["state"] == "error"): - any_failure = True - if not as_json: - print( - f"# skills ({len(skills)} declared under {root}; " - f"{counts['ok']} ok, {counts['warn']} warn, {counts['error']} error)" - ) - for row in report["skills"]: - if row["error"] is not None: - state = f"ERROR: {row['error']}" - elif row["state"] == "ok": - state = "ok" - else: - worst = next(f for f in row["findings"] if f["level"] == row["state"]) - state = f"{'ERROR' if row['state'] == 'error' else 'warn'}: {worst['message']}" - print( - f"{row['name']}\t{row['manifest_status'] or '-'}\t{row['path'] or '-'}\t{state}" - ) - for f in row["findings"]: - print(f" {f['level']} {f['code']}: {f['message']}") - if row["disable_model_invocation"]: - print( - " note disable-model-invocation: true (the model is never offered this skill)" - ) - print() - + any_failure = _scan_skills(args, root, report, as_json) if args.kind in ("tasks", "all"): - try: - tasks = read_task_manifest(root, tasks_subdir=args.tasks_subdir) - except ValueError as exc: - raise SystemExit(f"suites.json: {exc}") from exc - if not as_json: - print(f"# tasks ({len(tasks)} declared under {root})") - for task in tasks: - if task.loaded: - state = "ok" - elif task.skipped: - state = f"skipped: {task.reason}" - else: - state = f"ERROR: {task.reason}" - any_failure = True - report["tasks"].append( - { - "name": task.name, - "manifest_status": None if task.status is None else str(task.status), - "track": task.track, - "path": str(task.path) if task.path else None, - "state": "ok" if task.loaded else "skipped" if task.skipped else "error", - "reason": task.reason, - } - ) - if not as_json: - print(f"{task.name}\t{task.status or '-'}\t{task.track or '-'}\t{state}") - + any_failure = _scan_tasks(args, root, report, as_json) or any_failure if as_json: print(json.dumps(report, indent=2)) - if any_failure: raise SystemExit(1) diff --git a/dokimi/dokimi/external/catalog.py b/dokimi/dokimi/external/catalog.py index 2c6b6098b..92c23ec9b 100644 --- a/dokimi/dokimi/external/catalog.py +++ b/dokimi/dokimi/external/catalog.py @@ -93,40 +93,45 @@ def loaded(self) -> bool: return self.task is not None and not self.skipped and not self.failed -def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: - """Read skills under *root*, preferring a ``skills.yaml`` manifest.""" +def _scan_skill_files(root: Path) -> list[SkillCatalogEntry]: + """One entry per ``SKILL.md`` under *root*, as the manifest path gives. - root = Path(root) - manifest_path = root / "skills.yaml" - if not manifest_path.is_file(): - # One entry per SKILL.md, as the manifest path gives: a skill that does not - # load is an error row, so one broken skill does not abort the whole scan. - found: list[SkillCatalogEntry] = [] - for skill_file in sorted(root.rglob("SKILL.md")): - try: - skill = load_skill(skill_file) - except (OSError, ValueError) as exc: - found.append( - SkillCatalogEntry( - name=skill_file.parent.name, - status=None, - path=skill_file.parent, - source_root=root, - error=str(exc), - ) - ) - continue + A skill that does not load is an error row, so one broken skill does not abort the scan. + """ + found: list[SkillCatalogEntry] = [] + for skill_file in sorted(root.rglob("SKILL.md")): + try: + skill = load_skill(skill_file) + except (OSError, ValueError) as exc: found.append( SkillCatalogEntry( - name=skill.name, + name=skill_file.parent.name, status=None, - path=skill.path.parent, + path=skill_file.parent, source_root=root, - skill=skill, + error=str(exc), ) ) - return found + continue + found.append( + SkillCatalogEntry( + name=skill.name, + status=None, + path=skill.path.parent, + source_root=root, + skill=skill, + ) + ) + return found + + +def _manifest_items(manifest_path: Path) -> tuple[list, bool]: + """The entries of ``skills.yaml`` and whether the file is the list shape. + uxlfoundation/skills nests the entries under ``skills:``, each with a required + catalog_path; intel/skills makes the whole file the list and keeps every skill at + skills/. + """ import yaml try: @@ -134,9 +139,6 @@ def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: except yaml.YAMLError as exc: raise ValueError(f"invalid skills.yaml at {manifest_path}: {exc}") from exc - # uxlfoundation/skills nests the entries under ``skills:``, each with a - # required catalog_path; intel/skills makes the whole file the list and - # keeps every skill at skills/. if isinstance(data, dict): items = data.get("skills") if items is None: @@ -155,7 +157,18 @@ def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: f"invalid skills.yaml at {manifest_path}: 'skills' must be a list, " f"got {type(items).__name__}" ) + return items, list_shape + + +def read_skill_manifest(root: Path) -> list[SkillCatalogEntry]: + """Read skills under *root*, preferring a ``skills.yaml`` manifest.""" + + root = Path(root) + manifest_path = root / "skills.yaml" + if not manifest_path.is_file(): + return _scan_skill_files(root) + items, list_shape = _manifest_items(manifest_path) entries: list[SkillCatalogEntry] = [] for index, item in enumerate(items): if not isinstance(item, dict): diff --git a/dokimi/scripts/growth_baseline.json b/dokimi/scripts/growth_baseline.json index 974dd0e6f..7f3a9515e 100644 --- a/dokimi/scripts/growth_baseline.json +++ b/dokimi/scripts/growth_baseline.json @@ -20,7 +20,6 @@ "dokimi/commands/arms.py::_validate_expected_skills": {"complexity": 11}, "dokimi/commands/benchmark.py::cmd_benchmark_batch": {"complexity": 11}, "dokimi/commands/benchmark.py::cmd_benchmark_preflight": {"complexity": 22}, - "dokimi/commands/catalog.py::cmd_catalog_scan": {"complexity": 12}, "dokimi/commands/catalog.py::cmd_catalog_trigger_set": {"complexity": 18}, "dokimi/commands/evaluate.py::cmd_eval_judge_validity": {"complexity": 29, "rationale": "grew on main in #73 (judge-validity --context stored) from 25 to 29 before the ratchet landed; split it in its own PR"}, "dokimi/commands/judge_reliability.py::cmd_eval_cross_judge": {"complexity": 32, "rationale": "pre-existing when the gate was introduced"}, From 29e767f6911c305ddd9dda15425f8076b5f022fa Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Fri, 23 Oct 2026 12:45:34 -0700 Subject: [PATCH 08/11] fix(catalog): compare skill paths resolved so a symlinked root does not crash the duplicate check; pin root-skill rules Signed-off-by: Nikolay Petrov --- dokimi/dokimi/external/validate.py | 15 +++++- dokimi/tests/test_catalog_validate.py | 74 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index c9e068c56..d46ffe371 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -35,6 +35,7 @@ import hashlib import re from dataclasses import dataclass, field +from pathlib import Path from typing import Optional, Sequence import yaml @@ -165,6 +166,16 @@ def _unloaded_review(entry: SkillCatalogEntry) -> Optional[SkillReview]: return None if parse_error is None else SkillReview(findings=[parse_error]) +def _relative_to_root(path: Path, root: Path) -> str: + """``path`` relative to ``root`` (compared as written, then resolved), else as written.""" + for a, b in ((path, root), (path.resolve(), root.resolve())): + try: + return a.relative_to(b).as_posix() + except ValueError: + continue + return path.as_posix() + + def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillReview]]: """One :class:`SkillReview` per entry, in order. @@ -180,13 +191,13 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe continue skill = entry.skill findings, hidden = _frontmatter_findings( - skill, at_root=skill.path.parent == entry.source_root + skill, at_root=skill.path.parent.resolve() == entry.source_root.resolve() ) review = SkillReview(findings=findings, disable_model_invocation=hidden) review.content_sha256 = _skill_digest(skill) original = first_by_digest.setdefault(review.content_sha256, skill) if original is not skill: - where = original.path.parent.relative_to(entry.source_root).as_posix() + where = _relative_to_root(original.path.parent, entry.source_root) review.duplicate_of = where review.findings.append( Finding( diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 785a7453f..9b5500325 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -344,3 +344,77 @@ def test_json_tasks_array_carries_loaded_and_skipped_rows(tmp_path, capsys): assert rows["real"]["state"] == "ok" and rows["real"]["track"] == "executable" assert rows["real"]["manifest_status"] == "implemented" assert rows["later"]["state"] == "skipped" and rows["later"]["reason"] + + +def _root_skill(root: Path, frontmatter: str) -> None: + root.mkdir(exist_ok=True) + (root / "SKILL.md").write_text(f"---\n{frontmatter}---\n# z\n", encoding="utf-8") + + +def test_root_skill_still_gets_the_name_format_check(tmp_path, capsys): + root = tmp_path / "checkout" + _root_skill(root, "name: Z_z\ndescription: Use for z.\n") + report, _ = _scan_json(root, capsys) + + assert [f["code"] for f in report["skills"][0]["findings"]] == ["name-format"] + + +def test_root_skill_without_a_name_does_not_cite_the_checkout_directory(tmp_path, capsys): + root = tmp_path / "checkout" + _root_skill(root, "description: Use for z.\n") + report, _ = _scan_json(root, capsys) + + [finding] = report["skills"][0]["findings"] + assert finding["code"] == "name-missing" + assert "directory name" not in finding["message"] + assert "checkout" not in finding["message"] + + +def test_a_skill_without_a_manifest_status_has_a_null_status_in_json(tmp_path, capsys): + _skill(tmp_path / "skills", "a", name="a") + report, _ = _scan_json(tmp_path, capsys) + + assert report["skills"][0]["manifest_status"] is None + + +def test_a_task_without_a_manifest_status_has_a_null_status_in_json(tmp_path, capsys): + _skill(tmp_path / "skills", "s", name="s") + (tmp_path / "skills.yaml").write_text( + 'skills:\n - name: s\n catalog_path: skills/s\n', encoding="utf-8") + (tmp_path / "evaluation" / "harbor").mkdir(parents=True) + (tmp_path / "evaluation" / "harbor" / "suites.json").write_text( + json.dumps({"schema_version": "1.0", "suites": [{"skill": "s", "tasks": [ + {"name": "later", "track": "executable"}]}]}), + encoding="utf-8", + ) + report, _ = _scan_json(tmp_path, capsys, kind="tasks") + + assert [t["manifest_status"] for t in report["tasks"]] == [None] + + +def test_a_duplicate_found_through_a_symlinked_root_is_a_row_not_a_traceback(tmp_path, capsys): + real = tmp_path / "real" + for name in ("a", "b"): + _skill(real / "skills", name, name="same") + (real / "skills.yaml").write_text( + f"skills:\n - name: a\n catalog_path: {real / 'skills' / 'a'}\n" + " - name: b\n catalog_path: skills/b\n", encoding="utf-8") + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + report, _ = _scan_json(link, capsys) + + second = next(s for s in report["skills"] if s["duplicate_of"]) + assert second["duplicate_of"].endswith("skills/a") + assert "duplicate" in {f["code"] for f in second["findings"]} + + +def test_a_root_skill_reached_by_an_equivalent_spelling_skips_the_directory_check( + tmp_path, capsys): + root = tmp_path / "checkout" + _root_skill(root, "name: zzz\ndescription: Use for z.\n") + (root / "sub").mkdir() + (root / "skills.yaml").write_text( + "skills:\n - name: zzz\n catalog_path: sub/..\n", encoding="utf-8") + report, _ = _scan_json(root, capsys) + + assert report["skills"][0]["findings"] == [] From f3c8fbc572a60772e9eae8a9fb4fbd093181cc8d Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Sat, 24 Oct 2026 23:23:42 -0700 Subject: [PATCH 09/11] fix(catalog): leave out templates and keep discover_skills' mirror original in scan The manifest-less scan replaced discover_skills with its own rglob, so on top of main's template/mirror handling it listed template scaffolds again (failing --strict on anthropics/skills) and flagged as the duplicate the copy the skill-catalog arm keeps. Skip TEMPLATE_DIR_NAMES and pick the fewest-parts copy. Signed-off-by: Alexander Rybkin --- ...50000_napetrov_catalog-scan-spec-review.md | 6 +-- dokimi/docs/external-content.md | 7 ++- dokimi/dokimi/external/catalog.py | 5 +++ dokimi/dokimi/external/validate.py | 43 +++++++++++-------- dokimi/tests/test_catalog_validate.py | 22 ++++++++++ 5 files changed, 61 insertions(+), 22 deletions(-) diff --git a/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md index 61aee1ae4..21585b12a 100644 --- a/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md +++ b/dokimi/changelog.d/20261021_150000_napetrov_catalog-scan-spec-review.md @@ -3,8 +3,8 @@ - **`catalog scan` reviews each skill against the Agent Skills spec** — a loaded skill whose `name` is not its directory name or not kebab-case (at most 64 characters), or whose `description` is over 1024 characters, is an `error`; a `name` missing from the - frontmatter, or a `SKILL.md` byte-identical to an earlier skill's (a mirror, named - with its first copy), is a `warn`. `disable-model-invocation: true` is printed as a + frontmatter, or a `SKILL.md` byte-identical to another skill's (a mirror, named with + the copy nearest the root, the one the `skill-catalog:` arm offers), is a `warn`. `disable-model-invocation: true` is printed as a note, since the model is never offered that skill. Each finding is printed under its skill. - **`catalog scan --strict`** — exits 1 on a skill spec error as well as on a skill that @@ -21,4 +21,4 @@ before. `parse_frontmatter` is unchanged for other callers. - **`catalog scan` without a `skills.yaml` no longer stops at the first broken skill** — each `SKILL.md` under the root is an entry, so a skill that does not load is an error row and - the rest are still reviewed. + the rest are still reviewed. A `SKILL.md` under `template/` or `templates/` is still left out. diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index b49aa3394..78302a4fa 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -50,7 +50,8 @@ way the readers expect. Substitute your own checkout path. ``` Both manifests are optional — with no manifest, the readers fall back to a -plain filesystem scan of the same directories. `skills.yaml` comes in two +plain filesystem scan of the same directories (a `SKILL.md` under a `template/` +or `templates/` directory is a scaffold and is left out). `skills.yaml` comes in two shapes, and both hold a *list* of entries: ```yaml @@ -121,7 +122,9 @@ its directory name and kebab-case (at most 64 characters; a `SKILL.md` directly at most 1024 characters: a violation is an `error`, printed under the skill. It exits 1 only with `--strict`; without it, the scan reports the error and still exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to -an earlier skill's, is a `warn` that names the first copy; warnings never change +another skill's, is a `warn`. Of identical copies, the one nearest the root (fewest +path parts, ties to path order) is the original, the copy the `skill-catalog:` arm +offers; each other copy is the `warn` and names it. Warnings never change the exit code. `disable-model-invocation: true` is printed as a `note`, because the model is never offered that skill. diff --git a/dokimi/dokimi/external/catalog.py b/dokimi/dokimi/external/catalog.py index 92c23ec9b..b52e88120 100644 --- a/dokimi/dokimi/external/catalog.py +++ b/dokimi/dokimi/external/catalog.py @@ -30,6 +30,7 @@ from dokimi.harnesses.tasks import TaskSpec, discover_tasks, is_task_dir, load_task from dokimi.skills import Skill, load_skill +from dokimi.skills.loader import TEMPLATE_DIR_NAMES DEFAULT_TASKS_SUBDIR = "evaluation/harbor/tasks" DEFAULT_SUITES_MANIFEST = "evaluation/harbor/suites.json" @@ -97,9 +98,13 @@ def _scan_skill_files(root: Path) -> list[SkillCatalogEntry]: """One entry per ``SKILL.md`` under *root*, as the manifest path gives. A skill that does not load is an error row, so one broken skill does not abort the scan. + A ``SKILL.md`` under a ``template``/``templates`` directory is a scaffold and is left + out, as :func:`dokimi.skills.discover_skills` leaves it out. """ found: list[SkillCatalogEntry] = [] for skill_file in sorted(root.rglob("SKILL.md")): + if TEMPLATE_DIR_NAMES.intersection(skill_file.relative_to(root).parts[:-1]): + continue try: skill = load_skill(skill_file) except (OSError, ValueError) as exc: diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index d46ffe371..49e2517fd 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -18,8 +18,10 @@ description is a load error, raised by the loader. - The frontmatter is valid YAML (``frontmatter-parse``, ``error``). -Identical copies: a ``SKILL.md`` whose bytes are the same as an earlier -skill's is a ``warn`` naming that earlier skill. The model sees only the name and +Identical copies: of the skills whose ``SKILL.md`` bytes are the same, the one +nearest the root (fewest path parts, ties to path order) is the original, the +copy :func:`dokimi.skills.discover_skills` keeps; each other copy is a ``warn`` +naming it. The model sees only the name and description, so two such skills compete for the same query even when their bundled files differ (a plugin copy with an extra ``agents/`` folder is still a mirror). Mirrors are common (one skill checked in under two plugin directories), and @@ -35,7 +37,7 @@ import hashlib import re from dataclasses import dataclass, field -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Optional, Sequence import yaml @@ -180,11 +182,11 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe """One :class:`SkillReview` per entry, in order. None for an entry that did not load, unless its frontmatter is broken YAML: then its - review carries the ``frontmatter-parse`` error. The first skill of each identical - ``SKILL.md`` is the original; a later copy names it in ``duplicate_of``. + review carries the ``frontmatter-parse`` error. Of each identical ``SKILL.md``, the + copy nearest the root is the original; every other copy names it in ``duplicate_of``. """ reviews: list[Optional[SkillReview]] = [] - first_by_digest: dict[str, Skill] = {} + copies: list[tuple[SkillReview, str, str]] = [] # (review, skill directory, digest) per loaded skill for entry in entries: if not entry.loaded or entry.skill is None: reviews.append(_unloaded_review(entry)) @@ -195,16 +197,23 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe ) review = SkillReview(findings=findings, disable_model_invocation=hidden) review.content_sha256 = _skill_digest(skill) - original = first_by_digest.setdefault(review.content_sha256, skill) - if original is not skill: - where = _relative_to_root(original.path.parent, entry.source_root) - review.duplicate_of = where - review.findings.append( - Finding( - "warn", - "duplicate", - f"SKILL.md is identical to {where}; the same skill is counted twice", - ) - ) + where = _relative_to_root(skill.path.parent, entry.source_root) + copies.append((review, where, review.content_sha256)) reviews.append(review) + + original_of: dict[str, tuple[SkillReview, str]] = {} + for copy, where, digest in sorted(copies, key=lambda c: (len(PurePosixPath(c[1]).parts), c[1])): + original_of.setdefault(digest, (copy, where)) + for copy, _, digest in copies: + original, where = original_of[digest] + if original is copy: + continue + copy.duplicate_of = where + copy.findings.append( + Finding( + "warn", + "duplicate", + f"SKILL.md is identical to {where}; the same skill is counted twice", + ) + ) return reviews diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 9b5500325..b3e8b1ee7 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -13,6 +13,7 @@ from dokimi.cli import build_parser from dokimi.external import read_skill_manifest, review_skills +from dokimi.skills import discover_skills from dokimi.utils import parse_frontmatter, parse_frontmatter_strict @@ -418,3 +419,24 @@ def test_a_root_skill_reached_by_an_equivalent_spelling_skips_the_directory_chec report, _ = _scan_json(root, capsys) assert report["skills"][0]["findings"] == [] + + +def test_a_scan_without_a_manifest_leaves_out_a_template_as_discover_skills_does( + tmp_path, capsys): + _skill(tmp_path / "skills", "foo", name="foo") + _skill(tmp_path / "templates", "bar", name="Bar Template") + report, code = _scan_json(tmp_path, capsys, strict=True) + + assert [s["name"] for s in report["skills"]] == ["foo"] + assert code == 0 + + +def test_a_mirror_names_as_original_the_copy_discover_skills_keeps(tmp_path): + _skill(tmp_path / "plugin" / "skills", "foo", name="foo") + _skill(tmp_path / "skills", "foo", name="foo") + entries = read_skill_manifest(tmp_path) + duplicate_of = {e.path.relative_to(tmp_path).as_posix(): r.duplicate_of + for e, r in zip(entries, review_skills(entries), strict=True)} + + assert [s.path.parent for s in discover_skills(tmp_path)] == [tmp_path / "skills" / "foo"] + assert duplicate_of == {"plugin/skills/foo": "skills/foo", "skills/foo": None} From 0b920367dc7ac858535cfe0fdff7d3011b7fcaf9 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Sat, 24 Oct 2026 23:24:21 -0700 Subject: [PATCH 10/11] fix(catalog): do not quote a SKILL.md outside the root in the frontmatter-parse review A skill that fails to load never reaches _skill_entry's "resolves outside root" check, so _unloaded_review read a symlinked SKILL.md outside --root and put a line of it into the text and JSON report. Check containment before reading. Signed-off-by: Alexander Rybkin --- dokimi/dokimi/external/validate.py | 10 ++++++++-- dokimi/tests/test_catalog_validate.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 49e2517fd..6c7cccf49 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -45,7 +45,7 @@ from dokimi.skills import Skill from dokimi.utils import parse_frontmatter_strict -from .catalog import SkillCatalogEntry +from .catalog import SkillCatalogEntry, _within NAME_PATTERN = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*") NAME_MAX = 64 @@ -154,12 +154,18 @@ def _skill_digest(skill: Skill) -> str: def _unloaded_review(entry: SkillCatalogEntry) -> Optional[SkillReview]: - """A review for an entry that did not load, only when its ``SKILL.md`` has broken YAML; else None.""" + """A review for an entry that did not load, only when its ``SKILL.md`` has broken YAML; else None. + + A ``SKILL.md`` that resolves outside the scanned root is not read: the YAML message quotes + the offending line, and the catalog must not get a file outside it into the report. + """ if entry.path is None: return None candidate = entry.path / "SKILL.md" if entry.path.is_dir() else entry.path if candidate.name != "SKILL.md" or not candidate.is_file(): return None + if not _within(candidate, entry.source_root): + return None try: text = candidate.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index b3e8b1ee7..6026ad216 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -440,3 +440,18 @@ def test_a_mirror_names_as_original_the_copy_discover_skills_keeps(tmp_path): assert [s.path.parent for s in discover_skills(tmp_path)] == [tmp_path / "skills" / "foo"] assert duplicate_of == {"plugin/skills/foo": "skills/foo", "skills/foo": None} + + +@pytest.mark.parametrize("manifest", [True, False]) +def test_broken_yaml_outside_the_root_is_not_quoted_in_the_report(tmp_path, capsys, manifest): + outside = tmp_path / "private.md" + outside.write_text("---\napi_token: sk-SECRET-12345: x\n---\nbody\n", encoding="utf-8") + root = tmp_path / "cat" + (root / "skills" / "a").mkdir(parents=True) + (root / "skills" / "a" / "SKILL.md").symlink_to(outside) + if manifest: + (root / "skills.yaml").write_text("- name: a\n", encoding="utf-8") + report, code = _scan_json(root, capsys) + + assert "SECRET" not in json.dumps(report) + assert report["skills"][0]["state"] == "error" and code == 1 From 4e295cabb35e00bce1d2a8d37195aa6f7d0eb89f Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Sat, 24 Oct 2026 23:24:49 -0700 Subject: [PATCH 11/11] test(catalog): pin the default-kind exit code and the unreadable-skill error row Dropping `or any_failure` let a default `catalog scan` on a broken skill exit 0, and narrowing the OSError catch back to FileNotFoundError brought back the PermissionError traceback; the suite stayed green on both. Signed-off-by: Alexander Rybkin --- dokimi/tests/test_catalog_validate.py | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 6026ad216..2313223a6 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -455,3 +455,39 @@ def test_broken_yaml_outside_the_root_is_not_quoted_in_the_report(tmp_path, caps assert "SECRET" not in json.dumps(report) assert report["skills"][0]["state"] == "error" and code == 1 + + +def test_default_kind_exits_1_on_a_broken_skill_and_no_tasks(tmp_path, capsys): + (tmp_path / "skills" / "bad").mkdir(parents=True) + (tmp_path / "skills" / "bad" / "SKILL.md").write_text("---\nname: bad\n---\n", encoding="utf-8") + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path)]) + with pytest.raises(SystemExit) as exc: + args.func(args) + + assert exc.value.code == 1 + assert "ERROR" in capsys.readouterr().out + + +@pytest.mark.parametrize("manifest", [True, False]) +def test_an_unreadable_skill_is_an_error_row_and_the_rest_are_listed( + tmp_path, capsys, monkeypatch, manifest): + import dokimi.external.catalog as external_catalog + + real_load = external_catalog.load_skill + + def load(path): + if "b" in Path(path).parts: + raise PermissionError(13, "Permission denied", str(path)) + return real_load(path) + + monkeypatch.setattr(external_catalog, "load_skill", load) + for name in ("a", "b"): + _skill(tmp_path / "skills", name, name=name) + if manifest: + (tmp_path / "skills.yaml").write_text("- name: a\n- name: b\n", encoding="utf-8") + report, code = _scan_json(tmp_path, capsys) + + states = {s["name"]: (s["state"], s["error"]) for s in report["skills"]} + assert states["a"] == ("ok", None) + assert states["b"][0] == "error" and "Permission denied" in states["b"][1] + assert code == 1