feat(catalog): review skills against the Agent Skills spec, add --strict and --format json by napetrov · Pull Request #163 · intel/dokimi

napetrov · 2026-10-22T05:34:14Z

Part of #4.

Summary

catalog scan now reviews each loaded skill against the Agent Skills spec, and the report is machine-readable.

 catalog scan --root <catalog>
   read_skill_manifest(root)              # loader, unchanged in shape
+  review_skills(skills)                  # new: external/validate.py
+    name == directory name, kebab-case, <= 64 chars   -> error
+    description <= 1024 chars                          -> error
+    name missing from frontmatter                      -> warn
+    SKILL.md byte-identical to an earlier skill        -> warn (names first copy)
+    disable-model-invocation: true                     -> note, not a finding
+  exit 1 on a load error                  (as before)
+  exit 1 on a spec error only with --strict
+  --format json                           one object: state, findings, content_sha256, duplicate_of

Spec errors are reported and do not change the exit code unless --strict is given, so existing scripts keep their exit status. Broken YAML frontmatter is named (frontmatter-parse, with the YAML message) instead of showing as a missing description. Without a skills.yaml, the scan now reviews every SKILL.md under the root, and a skill that does not load becomes an error row instead of aborting the scan.

Evidence

The new tests live in tests/test_catalog_validate.py (17 tests). The CLI-level tests call the catalog scan command, so they show the old behaviour directly.

before: commands/catalog.py reverted to intel/main, rest of this branch kept
  FAILED test_strict_makes_a_spec_error_exit_1
  FAILED test_strict_does_not_fail_on_warnings
  FAILED test_json_report_on_warnings_only_exits_0
  FAILED test_json_report_keeps_load_errors_as_error_rows
  FAILED test_json_report_carries_the_findings_and_exits_0_on_a_spec_error_without_strict
  FAILED test_text_output_lists_each_finding_under_its_skill
  FAILED test_broken_yaml_is_reported_in_place_of_the_missing_description_error
  7 failed, 10 passed

after: this branch, full suite below (includes tests/test_catalog_validate.py)

Local checks on the rebased head:

uv run ruff check .                              All checks passed!
uv run mypy dokimi                               Success: no issues found in 223 source files
uv run python -m dokimi.schema_check             exit 0
uv run python -m dokimi.config_check             exit 0
uv run pytest -n auto -q                         6896 passed, 22 skipped

Merge Danger

Door: two-way. The new flags and the JSON object are additive, and the exit code without --strict is unchanged for every case that exited 0 before. Reverting the commits restores the old scan.

Blast Radius: low, confined to catalog scan.

Behaviour changes for callers:

Per-skill error rows. Without --strict, a spec error prints as an ERROR row and the scan still exits 0. Users who want a failing gate must pass --strict.
Text output. The header now reads # skills (N declared under ...; x ok, y warn, z error), and finding lines are indented under their skill. Parsers of the old header line need updating.
Scan without skills.yaml. It now enumerates every SKILL.md under the root (rglob), so vendored or mirrored copies appear as rows, and mirrors are reported as warn.
Broken frontmatter. A SKILL.md with invalid YAML exits 1, with or without --strict, as a load error does.
No persisted-format or schema change. parse_frontmatter keeps its lenient behaviour; the strict variant is a new function used only by the validator.

Checklist

Tests added/updated and pytest passes
ruff, mypy, and schema checks pass
Docs updated: docs/external-content.md (scan section) and the README step-1 command
No ad-hoc run artifacts committed
New code and test files carry the SPDX license header (the changelog fragment follows the existing fragments, which carry none)

🤖 Generated with Claude Code

`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 <nikolay.a.petrov@intel.com>

`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 <nikolay.a.petrov@intel.com>

codecov-commenter · 2026-10-22T05:48:48Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.54839% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.97%. Comparing base (94e3eb1) to head (29e767f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
dokimi/dokimi/external/validate.py	91.20%	5 Missing and 3 partials ⚠️
dokimi/dokimi/commands/catalog.py	94.11%	3 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #163    +/-   ##
========================================
  Coverage   91.97%   91.97%            
========================================
  Files         223      224     +1     
  Lines       27709    27863   +154     
  Branches     4553     4574    +21     
========================================
+ Hits        25486    25628   +142     
- Misses       1574     1581     +7     
- Partials      649      654     +5

Files with missing lines	Coverage Δ
dokimi/dokimi/external/__init__.py	`100.00% <100.00%> (ø)`
dokimi/dokimi/external/catalog.py	`97.12% <100.00%> (+0.29%)`	⬆️
dokimi/dokimi/utils.py	`98.00% <100.00%> (+0.12%)`	⬆️
dokimi/dokimi/commands/catalog.py	`97.45% <94.11%> (-0.86%)`	⬇️
dokimi/dokimi/external/validate.py	`91.20% <91.20%> (ø)`

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

antonwolfy

Review at 1868f79. The loader changes and the exit-code split between load errors and --strict hold up. The problems are at the edges: one JSON crash, one false --strict failure, and tests that leave the name rules unpinned. Fix the first two inline comments before merge.

What I ran, on a Linux review host at 1868f79:

make ci-local: 6,897 tests passed, 21 skipped; the changed-line coverage step, schema-check, the changelog check, the wheel smoke tests and gitleaks pass. The ruff, mypy, deptry, bandit, zizmor, pre-commit, mkdocs and pip-audit steps could not run in my venv: make dev stops at the tools install on this base, which #179 fixed on main. Static checks, mypy and the strict docs build pass on GitHub's CI at this head.
Reproductions for the code findings, on fixture catalogs, with no model or network involved.
A mutation run on validate.py and commands/catalog.py: 80 mutants, 48 killed, 32 survive the 17 tests in test_catalog_validate.py (7 of the survivors are equivalent).

Test gaps (the code is correct today; these changes would pass the suite):

The name rules beyond one title-case name are not pinned. These mutants of validate.py:116 all pass the suite: > to >= on NAME_MAX, NAME_MAX = 200, the length check removed, a regex that allows a--b, a trailing -?, _ allowed, fullmatch replaced by match. A 65-character name, a--b, a-, a_b would then be ok. Suggestion: a parametrized test where name and directory match, so only name-format fires: "a"*64 and "a1-2b" give no finding; "a"*65, "a--b", "a-", "-a", "a_b", "a b" give ["name-format"].
Duplicate detection is not pinned to whole-file bytes and to the first copy. Hashing only the frontmatter, hashing stripped text, and "latest copy wins" all pass. The existing test changes the description, so the frontmatter differs. Suggestion: same frontmatter with a different body is not a duplicate; a trailing-newline difference is not a duplicate; three identical copies all name the first, which has duplicate_of is None.
state precedence and disable-model-invocation: false are untested. Swapping the warn and error ranks in SkillReview.state passes, so a mirror with a bad name would be warn and pass --strict. Reading the flag with bool(meta.get(...)) passes, so false would hide a skill. Suggestion: a bad-name skill that is also a mirror expects state == "error" and --strict exit 1; a disable-model-invocation: false skill expects the flag False.
The JSON tasks array has no content test. Every JSON test uses --kind skills or asserts report["tasks"] == []. Renaming track or reason, mapping skipped to ok, dropping the append, or printing the # tasks header in JSON mode all pass. Suggestion: one --kind all --format json run on a catalog with a loaded and a skipped task; parse the whole stdout and assert the task rows.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

antonwolfy

I pushed a commit per nit, up to 1ed1cfb, which fix the two nits from my review:

7e1d17e: dokimi/dokimi/external/catalog.py:108: catch OSError instead of FileNotFoundError in the SKILL.md scan and in _skill_entry (:204), keeping ValueError.
1ed1cfb: dokimi/docs/external-content.md:92: the example header now shows the ok/warn/error counts, and a sentence at :130 says the YAML message is on the frontmatter-parse line while the row's main line and JSON error still say the description is missing.

Left as is:

The four test-gap points are not labelled Nit: or Non-blocking: in the review body, so they stay with the author.

make ci-local at 1ed1cfb: 20 passed, 8 failed (all exit 127, tools missing on the host), 7 skipped.

Not approving yet. Still open: the bug and the two risks from my review.

napetrov · 2026-10-22T15:06:05Z

Test gaps from the review body, all added in 67b683e (tests/test_catalog_validate.py):

Name rules: test_valid_names_matching_the_directory_have_no_finding, test_malformed_names_matching_the_directory_only_fail_name_format (65 chars, a--b, a-, -a, a_b, a b).
Duplicates: test_same_frontmatter_with_a_different_body_is_not_a_duplicate, test_a_trailing_newline_difference_is_not_a_duplicate, test_three_identical_copies_all_name_the_first.
State and flag: test_an_error_outranks_a_warning_in_the_state_and_fails_strict, test_disable_model_invocation_false_does_not_hide_the_skill.
JSON tasks: test_json_tasks_array_carries_loaded_and_skipped_rows (--kind all, one loaded and one skipped task).
I did not re-run the mutation run.

…or 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 <nikolay.a.petrov@intel.com>

antonwolfy

Review at 67b683e. Both earlier threads are fixed and tested. One new crash in the duplicate check is worth fixing before merge, plus a test gap and a doc nit, three points in all, all inline.

What I ran, on a Linux review host at 67b683e:

make ci-local: 27 passed, 1 failed, 7 skipped. 6,933 tests passed, 1 skipped; the changed-line coverage step, schema-check, ruff, mypy, deptry, bandit, zizmor, the strict docs build, the wheel smoke tests and gitleaks pass. The one failure is pip-audit on the lock files, which this PR does not touch.
Reproductions for the code finding, on fixture catalogs, with no model or network involved.
A mutation run on validate.py and commands/catalog.py: 56 mutants, 41 killed, 15 survive. The name-rule, duplicate, state and --strict mutants are all killed now. The survivors that matter are in the test-gap comment.

Checked, no issues

JSON status: date, float and bool status values come out as strings in the skill and task rows, and --format json stdout parses as one object with and without --strict.
Root SKILL.md: with a plain scan and with catalog_path: ., name-mismatch is skipped and name-format still applies.
Exit codes: a non-UTF-8 file, a missing description and broken YAML each exit 1 as a load error. A spec error alone exits 0 without --strict.

antonwolfy · 2026-10-23T10:41:36Z

+        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()


A duplicate skill crashes the whole scan with ValueError when --root is a symlink and a skills.yaml entry uses an absolute catalog_path into the real directory.

original.path.parent.relative_to(entry.source_root) compares literal paths. The loader accepts the entry because it compares resolved paths, so the first copy's path is under the real directory while source_root is the symlink.

Repro: realB/skills/a and realB/skills/b hold byte-identical SKILL.md files; linkB is a symlink to realB; realB/skills.yaml lists catalog_path: <abs path of realB>/skills/a and catalog_path: skills/b. catalog scan --root linkB --kind skills raises:

File ".../validate.py", line 189, in review_skills ValueError: '.../realB/skills/a' is not in the subpath of '.../linkB'

No report is printed, not even for the skills that loaded. The trigger is narrow, but the failure is a traceback instead of a row.

The same literal comparison at :183 (skill.path.parent == entry.source_root) misses equivalent spellings: with catalog_path: sub/.. on a root SKILL.md named zzz, the row gets ERROR: name 'zzz' is not the directory name '..', while catalog_path: ./ is skipped correctly.

Suggestion: resolve both sides before comparing (skill.path.parent.resolve() == entry.source_root.resolve()), and in the duplicate branch fall back to str(original.path.parent) when relative_to raises ValueError.

Fixed in 29e767f. The root check now compares resolved paths, and the duplicate branch uses a helper that tries the literal then the resolved relative path and falls back to the path as written, so it can no longer raise. Tests: test_a_duplicate_found_through_a_symlinked_root_is_a_row_not_a_traceback and test_a_root_skill_reached_by_an_equivalent_spelling_skips_the_directory_check (the sub/.. case); both fail on the old code.

antonwolfy · 2026-10-23T10:41:36Z

+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"] == []
+


The root-skill test pins only the name-mismatch skip; the other root-specific code and the None guard on manifest_status can break with the suite green.

Mutation check, all pass the 33 tests in this file:

str(skill.status) without the None guard at commands/catalog.py:31: every skill without a manifest status would show "None" in JSON. The date test only covers the str() part. The same holds for the task row at :110.

At validate.py:121, skipping the name-format check when at_root is true: a root SKILL.md named Z_z would be ok.

The at_root branch of the name-missing message at validate.py:109 replaced by the directory-name text: the message would again cite the checkout directory name, which is what the fix removed.

Suggestion: extend this test, or add two, so the root SKILL.md has name: Z_z (expects ["name-format"] only) and another has no name (expects name-missing with no directory name in the message), and assert manifest_status is None for a skill with no manifest status in a JSON scan.

Added in 29e767f: test_root_skill_still_gets_the_name_format_check (Z_z -> only name-format), test_root_skill_without_a_name_does_not_cite_the_checkout_directory, and test_a_skill_without_a_manifest_status_has_a_null_status_in_json / test_a_task_without_a_manifest_status_has_a_null_status_in_json. These pin existing behavior, so they pass on the current code; I did not re-run the mutation run.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

antonwolfy

I pushed a commit per nit, up to 9ec812d, which fix the nit from my review:

9ec812d: dokimi/docs/external-content.md:120: added that a SKILL.md directly in the scanned root is not compared with the root directory's name.

make ci-local at 9ec812d: 27 passed, 1 failed (pip-audit, same as at the reviewed head), 7 skipped.

GitHub's Tests fail at 9ec812d on every Python version, in test_growth_ratchet.py: commands/catalog.py::cmd_catalog_scan has complexity 21 against its baseline of 12, and external/catalog.py::read_skill_manifest has 11 with no baseline entry (the cap is 10). The commit changes only external-content.md. The ratchet came to main (#93) after this PR's earlier green run, and the PR's changes to those two functions take them past it. Splitting the two functions makes Tests green again.

Not approving yet. Still open: the bug and the risk from my review.

napetrov · 2026-10-23T19:45:15Z

Tests were red on the growth ratchet (cmd_catalog_scan complexity 21 vs baseline 12, read_skill_manifest 11 vs cap 10). a3bb3c4 splits both into helpers (behaviour unchanged, the catalog tests pass) and drops the cmd_catalog_scan baseline entry. The branch was also merged with main in 5ec24f7.

…er the complexity ratchet Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

…ot crash the duplicate check; pin root-skill rules Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

github-actions · 2026-10-23T20:22:17Z

CLI interface change detected

This PR changes dokimi's user-facing CLI surface (commands and/or options):

~ `dokimi catalog scan` parameters changed:
    + added option `--format`
    + added option `--strict`

Updated on every push; the cli-interface-change label is on while the diff is non-empty.

napetrov added 2 commits October 21, 2026 22:32

napetrov requested review from antonwolfy, jharlow-intel, vchamarthi and xaleryb as code owners October 22, 2026 05:34

napetrov mentioned this pull request Oct 22, 2026

ci: no-growth ratchet for file length and function complexity #93

Merged

5 tasks done

antonwolfy reviewed Oct 22, 2026

View reviewed changes

Comment thread dokimi/dokimi/commands/catalog.py Outdated

Comment thread dokimi/dokimi/external/validate.py

Comment thread dokimi/dokimi/external/catalog.py Outdated

Comment thread dokimi/docs/external-content.md

antonwolfy added 2 commits October 22, 2026 12:34

Catch OSError when loading a skill so unreadable files become error rows

7e1d17e

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

Update scan example header and explain the broken-YAML row

1ed1cfb

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

antonwolfy reviewed Oct 22, 2026

View reviewed changes

antonwolfy reviewed Oct 23, 2026

View reviewed changes

Note that a root SKILL.md skips the directory-name check in docs

9ec812d

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

antonwolfy reviewed Oct 23, 2026

View reviewed changes

Merge remote-tracking branch 'intel/main' into un1023-163

5ec24f7

napetrov added 2 commits October 23, 2026 12:45

refactor(catalog): split cmd_catalog_scan and read_skill_manifest und…

a3bb3c4

…er the complexity ratchet Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

fix(catalog): compare skill paths resolved so a symlinked root does n…

29e767f

…ot crash the duplicate check; pin root-skill rules Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

github-actions Bot added the cli-interface-change Changes dokimi's user-facing CLI surface label Oct 23, 2026

Conversation

napetrov commented Oct 22, 2026

Summary

Evidence

Merge Danger

Checklist

Uh oh!

codecov-commenter commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

napetrov commented Oct 22, 2026

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

antonwolfy Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

napetrov Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

antonwolfy Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

napetrov Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

napetrov commented Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

CLI interface change detected

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Oct 22, 2026 •

edited

Loading