From e8bf858be714d03cef6b138d81f3aab9b7f72c43 Mon Sep 17 00:00:00 2001 From: Edward Cheng-I Wu <132531341+Imbad0202@users.noreply.github.com> Date: Mon, 31 Aug 2026 02:02:28 +0800 Subject: [PATCH] lint: skill-inventory parity across top-level dirs, skills/ symlinks, CLAUDE.md table, and marketplace.json (#809) (#810) Closes #809. check_skill_inventory_parity.py: on-disk /SKILL.md dirs are the authority; set-equality against skills/ symlinks, the CLAUDE.md Skills Overview table (exact unfenced H2, GFM header+separator, first-cell backticked names), and marketplace.json plugins[].skills; "N skills" count claims on plugin.json / marketplace.json / MODE_REGISTRY.md. check_spec_consistency.py derives its skill paths from disk at call time; table-row grammar single-sourced in _skill_lint. 60 mutation tests; wired into spec-consistency.yml and the pytest manifest. Dual-track pre-ship: /simplify (4 findings applied), /security-review (none), codex gpt-5.6-sol xhigh 7 rounds (12 findings fixed, round 7 clean). CHANGELOG also covers #805. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014U5nvjKy84twtB4VYsrex1 --- .github/workflows/spec-consistency.yml | 10 + CHANGELOG.md | 6 + scripts/_ci_pytest_manifest.toml | 4 + scripts/_skill_lint.py | 13 + scripts/check_skill_inventory_parity.py | 355 +++++++++++++++ scripts/check_spec_consistency.py | 23 +- scripts/check_version_consistency.py | 6 +- scripts/test_check_skill_inventory_parity.py | 438 +++++++++++++++++++ scripts/test_check_spec_consistency.py | 12 + 9 files changed, 854 insertions(+), 13 deletions(-) create mode 100644 scripts/check_skill_inventory_parity.py create mode 100644 scripts/test_check_skill_inventory_parity.py diff --git a/.github/workflows/spec-consistency.yml b/.github/workflows/spec-consistency.yml index 189a973b..4087d567 100644 --- a/.github/workflows/spec-consistency.yml +++ b/.github/workflows/spec-consistency.yml @@ -90,6 +90,16 @@ jobs: PYTHONPATH: scripts run: python3 scripts/check_data_access_level.py + - name: Check skill-inventory parity (#809) + # Top-level /SKILL.md dirs == skills/ symlinks == CLAUDE.md + # Skills Overview rows == marketplace.json plugins[].skills, plus the + # "N skills" count claims in plugin/marketplace descriptions. The pytest + # companion `test_check_skill_inventory_parity.py` runs via the unified + # manifest. + env: + PYTHONPATH: scripts + run: python3 scripts/check_skill_inventory_parity.py + - name: Check instruction-vs-data boundary (#272 guidance layer) # Drift guard for the retrieved-content instruction/data principle: # authoritative canonical block in ground_truth_isolation_pattern.md + diff --git a/CHANGELOG.md b/CHANGELOG.md index 977301f1..71e49342 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,14 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- **Skill-inventory parity lint (#809).** New `scripts/check_skill_inventory_parity.py` takes the top-level `/SKILL.md` directories as the authority and requires set-equality against the three surfaces that package or advertise the inventory: `skills/` symlinks (each must resolve to `../`), the `.claude/CLAUDE.md` Skills Overview table rows, and `.claude-plugin/marketplace.json` `plugins[].skills[]` (`./` form). It also checks that any "N skills" count claim on the three current-state metadata surfaces (`plugin.json` / `marketplace.json` descriptions, `MODE_REGISTRY.md`) equals the number of skills on disk; README and CHANGELOG are out of scope because their release notes carry legitimately frozen historical counts. The table-row grammar moves to `_skill_lint` (`SKILLS_TABLE_ROW_PREFIX` / `SKILLS_TABLE_ROW_FULL`) so this lint and `check_version_consistency.py` agree on what a row is, and a row that names a skill but lacks its `vX.Y.Z` token is reported here rather than silently skipped by the version lint; and `check_spec_consistency.py` now derives its skill list from disk instead of the hardcoded four-path tuple, so a new skill directory is version-policed the moment it exists. Motivation: triage of an external draft PR that added a fifth top-level skill directory showed the existing lints are anchored to the four skills they already know (`check_spec_consistency.py` hardcodes the paths; `check_version_consistency.py` iterates the CLAUDE.md table), so an unpackaged, unlisted skill passed every inventory lint. Wired into `spec-consistency.yml`; 60 mutation tests cover each surface in both directions, dangling / mis-targeted / non-symlink `skills/` entries, malformed manifest entries, and stale count claims. + ### Fixed +- **MLA key-rules line in `citation_format_switcher.md` no longer reads "No year in in-text" (#805, by @LeslieLi46).** Docs-only; the line now matches the in-text format documented above it. + - **`check_surface_form_parity` names the broken environment instead of blaming the manifest (#801 follow-up).** With the manifest file present but pyyaml unimportable, `_load_manifest` returned `None`, so the lint failed with "manifest … is present but empty / null / non-mapping" — a misdiagnosis pointing the reader at the wrong file (pyyaml is declared in `requirements-dev.txt`, so only a broken local environment can hit this). The missing-import case is now a distinct `_YamlUnavailableError` and the lint fails naming pyyaml and the `pip install -r requirements-dev.txt` remedy; regression test pins that the message names the environment, not the manifest shape. Also de-enumerated the hand-maintained dependency parenthetical in `docs/SETUP.md` / `docs/SETUP.zh-TW.md` Method line ("PyYAML + jsonschema" was already stale against the manifest it mirrors two lines above); both language files move together. - **`normalize_cn_title` strips outer wrapper marks only when they enclose the whole title as one balanced unit (#800).** The wrapper strip inherited from #431 (and promoted unchanged by #798/#799) was positional: it removed the first and last characters whenever they matched as a wrapper pair *type*, without checking they belonged to the same bracket pair. `《红楼梦》与《金瓶梅》` — two titles joined in one string — therefore normalized to `红楼梦》与《金瓶梅`, leaving an orphaned `》` mid-key. Matching correctness was never affected (both sides of every comparison pass through the same normalization, and no exploitable asymmetry was found in the #799 security pass), but the mangled key is a semantic anomaly for any future single-sided consumer (display, logging, cache keys, or comparison against an externally-normalized key). Fix adds `_outer_pair_encloses`: the outer marks are stripped only when the interior between them is itself balanced under all six wrapper pairs, so `《围城》` still strips to `围城` and nested balanced interiors (`《基于「ProEXC」的研究》` → `基于「ProEXC」的研究`) still unwrap, while `《红楼梦》与《金瓶梅》` and `“研究”与“实践”` keep their marks. The interior scan is scoped to the outer pair's **own family**: `’` is also the English apostrophe and `”` also appears unpaired, so a family-blind scan read the lone `’` in `《Alzheimer’s病中ＰｒｏＥＸＣ表达》` as an unbalanced quote and refused to strip a genuine `《…》` wrap — dropping a pair that matched before to exact=False and ratio 0.6818, below the 0.70 floor, which fails the DOI-keyed ratio gate and the title-fallback exact gate at once and is the failure class #798 repaired. Scoping costs the check nothing, since any mark that can orphan the outer pair is by definition of that pair's own family. Both consumers change together — the CJK client re-imports the shared function (#799), pinned behaviorally as well as by identity. Scope of the invariance claim, stated precisely: on the two `has_cjk`-gated paths (`exact_normalized_title`'s third branch and `_similarity`'s CJK fold) every verdict and ratio for titles without Han ideographs is unchanged, pinned by the #799 pre-fix oracles. The client's `_cn_titles_match` is **not** `has_cjk`-gated — it calls `normalize_cn_title` directly — so a mark-carrying title with no Han ideograph can change verdict there (`《Hamlet》and《Macbeth》` no longer matches a pre-mangled `Hamlet》and《Macbeth`); that path is DOI-keyed and Chinese-corpus-only in practice, so the narrowing is accepted rather than gated. Likewise the empty-wrapper guard is a property of the CJK branch specifically (`_cjk_titles_match` requires a non-empty key): `exact_normalized_title("《》", "《》")` remains True through the ungated base-normalization branch, as it did before this change. diff --git a/scripts/_ci_pytest_manifest.toml b/scripts/_ci_pytest_manifest.toml index 65ccad7a..a257c2d3 100644 --- a/scripts/_ci_pytest_manifest.toml +++ b/scripts/_ci_pytest_manifest.toml @@ -204,6 +204,10 @@ path = "scripts/test_check_risk_register.py" id = "756-data-access-level-pins" path = "scripts/test_check_data_access_level.py" +[[pytest]] +id = "809-skill-inventory-parity" +path = "scripts/test_check_skill_inventory_parity.py" + [[pytest]] id = "v3.10-182-verification-cache" path = "scripts/test_verification_cache.py" diff --git a/scripts/_skill_lint.py b/scripts/_skill_lint.py index d3371170..4c02c0e4 100644 --- a/scripts/_skill_lint.py +++ b/scripts/_skill_lint.py @@ -29,6 +29,19 @@ from pathlib import Path import yaml +# One row of the `.claude/CLAUDE.md` § "Skills Overview" table. The first cell is +# the backticked skill directory name followed by its `vX.Y.Z` token. Two forms +# are shared so the lints agree on what a row is: +# PREFIX — name only. check_skill_inventory_parity.py uses it to find every +# row that names a skill, so a row can never hide from the parity +# check by omitting its version. +# FULL — name + version. check_version_consistency.py parses versions with +# it; the parity lint reports any PREFIX row that is not also a FULL +# row, closing the gap where a version-less row is invisible to the +# version lint (it only iterates FULL matches). +SKILLS_TABLE_ROW_PREFIX = r"^\|\s*`([a-z0-9-]+)`" +SKILLS_TABLE_ROW_FULL = SKILLS_TABLE_ROW_PREFIX + r"\s+v([A-Za-z0-9.\-_+]+)\s*\|" + SKIP_DIRS = frozenset( {"shared", "scripts", "docs", ".git", ".github", "examples", ".local-plans", ".claude"} ) diff --git a/scripts/check_skill_inventory_parity.py b/scripts/check_skill_inventory_parity.py new file mode 100644 index 00000000..22516619 --- /dev/null +++ b/scripts/check_skill_inventory_parity.py @@ -0,0 +1,355 @@ +#!/usr/bin/env python3 +"""Lint: the skill inventory is identical on every surface that lists it (#809). + +Triage of an external PR that added a fifth top-level skill directory showed +that the existing inventory lints are anchored to the skills they already +know about: `check_spec_consistency.py` hardcodes four SKILL.md paths, +`check_version_consistency.py` iterates the `.claude/CLAUDE.md` table, and +nothing cross-checks the `skills/` symlink directory or the marketplace +manifest. A skill could therefore exist on disk without being packaged, +listed, or versioned, with every lint green. + +This lint takes the set of top-level `/SKILL.md` directories as the +authority (it is what exists) and requires set-equality against the three +surfaces that advertise or package the inventory: + + B. `skills/` — one symlink per skill, resolving to `/` + (plugin auto-discovery packages from here); + C. `.claude/CLAUDE.md` § "Skills Overview" table rows (the canonical + inventory other lints iterate); + D. `.claude-plugin/marketplace.json` `plugins[].skills[]` as `./` + (what symlink-blind importers read). + +It also checks that any " skills" count claim equals the number of skills +on disk, on the three CURRENT-STATE metadata surfaces that carry one: +`.claude-plugin/plugin.json` description, `marketplace.json` top-level and +per-plugin descriptions, and `MODE_REGISTRY.md`. README and CHANGELOG are deliberately out of scope: they +carry historical release notes whose counts are legitimately frozen at the +time of writing, so a prose-wide grep would flag correct history. A surface +that carries no count makes no claim and is not checked. + +Every asymmetric difference is reported in both directions, so a stale row +and an unpackaged directory are both single, named violations. +""" +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path + +from _skill_lint import ( + SKILLS_TABLE_ROW_FULL, + SKILLS_TABLE_ROW_PREFIX, + heading_section, + iter_skill_files, +) + +SKILLS_DIR = "skills" +CLAUDE_MD = Path(".claude") / "CLAUDE.md" +MARKETPLACE_JSON = Path(".claude-plugin") / "marketplace.json" +PLUGIN_JSON = Path(".claude-plugin") / "plugin.json" +MODE_REGISTRY_MD = Path("MODE_REGISTRY.md") + +SKILLS_OVERVIEW_HEADING = "## Skills Overview" +# Row grammar shared with check_version_consistency.py. FULL is what the version +# lint iterates; a row that names a skill but is not FULL is reported here (the +# version lint would silently skip it). PREFIX is kept imported so the shared +# grammar has a visible second consumer. +TABLE_ROW_FULL_RE = re.compile(SKILLS_TABLE_ROW_FULL) +# Any backticked first cell at all, so a row with a non-canonical name (e.g. +# `Ghost-Skill`) is reported instead of being invisible to both lints. +TABLE_ROW_ANY_RE = re.compile(r"^\|\s*`([^`]+)`") +CANONICAL_NAME_RE = re.compile(r"[a-z0-9-]+") +# Markdown table separator row: `|---|:---:|`, `|:-|` etc. (GFM: one or more dashes). +TABLE_SEPARATOR_RE = re.compile(r"^\|\s*:?-+:?\s*(\|\s*:?-+:?\s*)*\|?\s*$") +# Marketplace skill paths are relative, `./`, one segment. +MANIFEST_SKILL_RE = re.compile(r"\./([a-z0-9-]+)") # used with fullmatch: `$` would admit a trailing newline +# A count claim such as "4 skills" / "4 Skills" (word-bounded so "40 skillsets" +# is not one; case-insensitive because these are prose surfaces). +COUNT_CLAIM_RE = re.compile(r"\b(\d+) skills\b", re.IGNORECASE) + + +def _skills_on_disk(root: Path) -> set[str]: + return {p.parent.name for p in iter_skill_files(root)} + + +def _skills_dir_entries(root: Path, violations: list[str]) -> set[str]: + skills_dir = root / SKILLS_DIR + if not skills_dir.is_dir(): + violations.append(f"{skills_dir}: directory is missing") + return set() + names: set[str] = set() + for entry in sorted(skills_dir.iterdir(), key=lambda p: p.name): + names.add(entry.name) + expected = root / entry.name + if not entry.is_symlink(): + violations.append( + f"{entry}: must be a symlink to ../{entry.name}, " + f"found a real {'directory' if entry.is_dir() else 'file'}" + ) + continue + try: + target = entry.resolve(strict=True) + except (FileNotFoundError, RuntimeError): + violations.append(f"{entry}: dangling symlink") + continue + if target != expected.resolve(): + violations.append( + f"{entry}: symlink resolves to {target}, expected {expected}" + ) + return names + + +def _overview_table_lines(section: str) -> list[str]: + """The ONE table that immediately follows the heading: skip leading blank + lines, then take the contiguous run of `|`-prefixed lines. A later table, + prose mention, or fenced Markdown sample in the same section is not the + inventory and must not satisfy the parity check.""" + lines = iter(section.splitlines()) + table: list[str] = [] + for line in lines: + if line.strip() == "": + if table: + break + continue + if not line.lstrip().startswith("|"): + break + table.append(line) + return table + + +def _cell_count(row: str) -> int: + """Cells in a GFM table row: strip one leading and one trailing pipe, + then split on unescaped pipes.""" + inner = row.strip() + inner = inner[1:] if inner.startswith("|") else inner + inner = inner[:-1] if inner.endswith("|") else inner + return len(re.split(r"(? set[str]: + claude_md = root / CLAUDE_MD + if not claude_md.is_file(): + violations.append(f"{claude_md}: file is missing") + return set() + text = claude_md.read_text(encoding="utf-8") + # Exact H2 line at column 0, outside code fences (shared helper): a + # demoted `### Skills Overview` or a copy inside a fenced example is not + # the section. + section = heading_section(text, SKILLS_OVERVIEW_HEADING) + if section is None: + violations.append( + f"{claude_md}: '{SKILLS_OVERVIEW_HEADING}' H2 is missing" + ) + return set() + rows: set[str] = set() + table = _overview_table_lines(section) + # A GFM table is header row + separator row + data rows. Both leading + # rows are required (a table without its separator is not a table, and + # the version lint would still read its rows). Every data row must carry + # a backticked skill name in its first cell; one that does not (e.g. + # `| ghost-skill v1.0.0 |`) is reported rather than dropped. + if len(table) < 2 or not TABLE_SEPARATOR_RE.match(table[1]): + violations.append( + f"{claude_md}: '{SKILLS_OVERVIEW_HEADING}' is not followed by a GFM " + f"table (header row then a |---| separator row)" + ) + return set() + header_cells, separator_cells = _cell_count(table[0]), _cell_count(table[1]) + if header_cells != separator_cells: + # GFM: header and delimiter rows must have the same cell count or + # GitHub does not render a table at all. Data-row width is NOT + # checked: GFM pads/truncates data rows, and this lint reads only + # the first cell, as check_version_consistency.py does. + violations.append( + f"{claude_md}: Skills Overview header has {header_cells} cells but " + f"its separator row has {separator_cells}; GFM requires them equal" + ) + return set() + data_rows = table[2:] + for line in data_rows: + any_row = TABLE_ROW_ANY_RE.match(line) + if any_row is None: + violations.append( + f"{claude_md}: Skills Overview data row {line.strip()!r} has no " + f"backticked skill name in its first cell" + ) + continue + name = any_row.group(1) + if CANONICAL_NAME_RE.fullmatch(name) is None: + violations.append( + f"{claude_md}: Skills Overview row names {name!r}, which is not " + f"a canonical skill directory name ([a-z0-9-]+); both lints " + f"would otherwise ignore this row" + ) + continue + rows.add(name) + if TABLE_ROW_FULL_RE.match(line) is None: + violations.append( + f"{claude_md}: Skills Overview row for '{name}' lacks a " + f"'vX.Y.Z' token after the name (check_version_consistency.py " + f"skips such rows, so the version would go unchecked)" + ) + if not rows: + violations.append( + f"{claude_md}: '{SKILLS_OVERVIEW_HEADING}' table has no " + f"backticked skill rows" + ) + return rows + + +def _load_json(path: Path, violations: list[str]) -> dict | None: + if not path.is_file(): + violations.append(f"{path}: file is missing") + return None + try: + data = json.loads(path.read_text(encoding="utf-8")) + except json.JSONDecodeError as exc: + violations.append(f"{path}: invalid JSON ({exc.msg} at line {exc.lineno})") + return None + if not isinstance(data, dict): + violations.append(f"{path}: top level must be an object") + return None + return data + + +def _marketplace_skills( + root: Path, violations: list[str] +) -> tuple[set[str], list[str]]: + """Return (skill names across all plugins, plugin descriptions).""" + path = root / MARKETPLACE_JSON + data = _load_json(path, violations) + if data is None: + return set(), [] + plugins = data.get("plugins") + if not isinstance(plugins, list) or not plugins: + violations.append(f"{path}: 'plugins' must be a non-empty list") + return set(), [] + names: set[str] = set() + descriptions: list[str] = [] + if isinstance(data.get("description"), str): + descriptions.append(data["description"]) # marketplace-level claim + for index, plugin in enumerate(plugins): + if not isinstance(plugin, dict): + violations.append(f"{path}: plugins[{index}] must be an object") + continue + description = plugin.get("description") + if isinstance(description, str): + descriptions.append(description) + skills = plugin.get("skills") + if not isinstance(skills, list): + violations.append( + f"{path}: plugins[{index}].skills must be a list of './' " + f"paths (symlink-blind importers read this list)" + ) + continue + for raw in skills: + match = MANIFEST_SKILL_RE.fullmatch(raw) if isinstance(raw, str) else None + if match is None: + violations.append( + f"{path}: plugins[{index}].skills entry {raw!r} must be " + f"'./' with a single lowercase path segment" + ) + continue + names.add(match.group(1)) + return names, descriptions + + +def _check_count_claim( + label: str, text: str, expected: int, violations: list[str] +) -> None: + for claimed in COUNT_CLAIM_RE.findall(text): + if int(claimed) != expected: + violations.append( + f"{label}: claims '{claimed} skills' but {expected} " + f"top-level skill directories exist" + ) + + +def _report_set_diff( + on_disk: set[str], other: set[str], surface: str, violations: list[str] +) -> None: + for name in sorted(on_disk - other): + violations.append( + f"skill '{name}' exists on disk (top-level {name}/SKILL.md) but " + f"is not listed in {surface}" + ) + for name in sorted(other - on_disk): + violations.append( + f"{surface} lists skill '{name}' but no top-level {name}/SKILL.md " + f"exists" + ) + + +def run_all_checks(root: Path) -> list[str]: + violations: list[str] = [] + on_disk = _skills_on_disk(root) + if not on_disk: + violations.append( + f"{root}: no top-level /SKILL.md found (wrong --path?)" + ) + return violations + + symlinked = _skills_dir_entries(root, violations) + _report_set_diff(on_disk, symlinked, f"{SKILLS_DIR}/ symlinks", violations) + + table = _claude_table_rows(root, violations) + _report_set_diff( + on_disk, table, f"{CLAUDE_MD} Skills Overview table", violations + ) + + manifest, market_descriptions = _marketplace_skills(root, violations) + _report_set_diff( + on_disk, manifest, f"{MARKETPLACE_JSON} plugins[].skills", violations + ) + for description in market_descriptions: + _check_count_claim( + f"{MARKETPLACE_JSON} description", description, len(on_disk), violations + ) + + plugin = _load_json(root / PLUGIN_JSON, violations) + if plugin is not None: + description = plugin.get("description") + if isinstance(description, str): + _check_count_claim( + f"{PLUGIN_JSON} description", description, len(on_disk), violations + ) + + registry = root / MODE_REGISTRY_MD + if registry.is_file(): + _check_count_claim( + str(MODE_REGISTRY_MD), + registry.read_text(encoding="utf-8"), + len(on_disk), + violations, + ) + else: + violations.append(f"{registry}: file is missing") + return violations + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument( + "--path", + type=Path, + default=Path(__file__).resolve().parent.parent, + ) + args = parser.parse_args() + violations = run_all_checks(args.path) + if violations: + for v in violations: + print(f"ERROR: {v}") + print(f"\n{len(violations)} violation(s) found.", file=sys.stderr) + return 1 + print( + "OK: skill inventory is identical across top-level directories, " + "skills/ symlinks, the CLAUDE.md table, and the marketplace manifest." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/check_spec_consistency.py b/scripts/check_spec_consistency.py index 53e125e7..91346740 100644 --- a/scripts/check_spec_consistency.py +++ b/scripts/check_spec_consistency.py @@ -13,11 +13,13 @@ if __package__: # Package import in tests. NON_RELATIVE_LINK_PREFIXES, extract_link_targets, ) + from ._skill_lint import iter_skill_files else: # pragma: no cover - exercised by the CLI smoke path from _markdown_lint_util import ( NON_RELATIVE_LINK_PREFIXES, extract_link_targets, ) + from _skill_lint import iter_skill_files ROOT = Path(__file__).resolve().parents[1] @@ -95,14 +97,17 @@ def check_claude_md() -> None: expect_absent(rel_path, forbidden) -# All four skills carry the same frontmatter (`version` / `last_updated`) + Version-Info-table -# (`| Skill Version |` / `| Last Updated |`) pair. Pre-#377 only the reviewer was policed. -_SKILL_VERSION_PATHS = ( - "academic-pipeline/SKILL.md", - "academic-paper/SKILL.md", - "academic-paper-reviewer/SKILL.md", - "deep-research/SKILL.md", -) +# Every top-level skill carries the same frontmatter (`version` / `last_updated`) + +# Version-Info-table (`| Skill Version |` / `| Last Updated |`) pair. Pre-#377 only the +# reviewer was policed. Derived from disk (#809) rather than hand-listed, so a new skill +# directory is policed the moment it exists; check_skill_inventory_parity.py pins that +# the on-disk set matches every surface that advertises it. +def _skill_version_paths() -> tuple[str, ...]: + """Read ROOT at call time (tests swap `csc.ROOT` for fixture trees; an + import-time tuple would carry the real checkout's skills into them).""" + return tuple( + f"{skill_md.parent.name}/SKILL.md" for skill_md in iter_skill_files(ROOT) + ) # The single skill whose `version` tracks the suite version. The other three move independently, # so only this one's date is sanity-checked against the release (CHANGELOG) in #377(b). @@ -139,7 +144,7 @@ def _parse_skill_version_block(rel_path: str) -> tuple[str, str, str, str] | Non def check_skill_version_blocks() -> None: """#377(a): for ALL FOUR SKILL.md, the frontmatter version/last_updated must match the Version-Info-table rows (an internal per-file consistency check).""" - for rel_path in _SKILL_VERSION_PATHS: + for rel_path in _skill_version_paths(): parsed = _parse_skill_version_block(rel_path) if parsed is None: continue diff --git a/scripts/check_version_consistency.py b/scripts/check_version_consistency.py index b45a937e..48122387 100644 --- a/scripts/check_version_consistency.py +++ b/scripts/check_version_consistency.py @@ -59,7 +59,7 @@ from pathlib import Path import yaml -from _skill_lint import parse_frontmatter, FrontmatterError +from _skill_lint import SKILLS_TABLE_ROW_FULL, parse_frontmatter, FrontmatterError # Broad token captures: anything that looks like an identifier inside the @@ -67,9 +67,7 @@ from _skill_lint import parse_frontmatter, FrontmatterError # token is a canonical semver. Using the regex as a filter (the pre-#169 # pattern) silently dropped invalid tokens and hid the very drift this lint # is meant to surface; see dual-track review on PR for that class of bug. -TABLE_TOKEN_RE = re.compile( - r"^\|\s*`([a-z0-9-]+)`\s+v([A-Za-z0-9.\-_+]+)\s*\|", re.MULTILINE -) +TABLE_TOKEN_RE = re.compile(SKILLS_TABLE_ROW_FULL, re.MULTILINE) SUITE_TOKEN_RE = re.compile( r"^\s*-\s*\*\*Suite version\*\*:\s*([A-Za-z0-9.\-_+]+)", re.MULTILINE ) diff --git a/scripts/test_check_skill_inventory_parity.py b/scripts/test_check_skill_inventory_parity.py new file mode 100644 index 00000000..0e9d7487 --- /dev/null +++ b/scripts/test_check_skill_inventory_parity.py @@ -0,0 +1,438 @@ +"""Mutation tests for check_skill_inventory_parity.py (#809). + +A synthetic root carries two skills on all four surfaces. Each test breaks +exactly one surface in one direction and asserts the lint names it. The +real repository tree is checked last. +""" +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from tests.test_helpers import run_skill_linter, write_skill + +from check_skill_inventory_parity import run_all_checks + +SCRIPT = Path(__file__).resolve().parent / "check_skill_inventory_parity.py" +REPO_ROOT = Path(__file__).resolve().parent.parent +NAMES = ("alpha-skill", "beta-skill") + + +def _write_table(root: Path, names: tuple[str, ...]) -> None: + rows = "\n".join(f"| `{n}` v1.0.0 | purpose | full |" for n in names) + (root / ".claude").mkdir(exist_ok=True) + (root / ".claude" / "CLAUDE.md").write_text( + "# Test\n\n## Skills Overview\n\n" + "| Skill | Purpose | Key Modes |\n|-------|---------|-----------|\n" + f"{rows}\n\n## Routing Rules\n\n" + "| `not-a-skill` v9.9.9 | a row outside the section | x |\n", + encoding="utf-8", + ) + + +def _write_manifests( + root: Path, + names: tuple[str, ...], + *, + market_entries: list[str] | None = None, + market_desc: str | None = None, + market_top_desc: str | None = None, + plugin_desc: str | None = None, +) -> None: + (root / ".claude-plugin").mkdir(exist_ok=True) + entries = market_entries if market_entries is not None else [f"./{n}" for n in names] + market = { + "name": "test", + "description": market_top_desc if market_top_desc is not None else "test marketplace", + "plugins": [ + { + "name": "test", + "description": market_desc if market_desc is not None else f"{len(names)} skills + modes", + "skills": entries, + } + ], + } + (root / ".claude-plugin" / "marketplace.json").write_text( + json.dumps(market, indent=2), encoding="utf-8" + ) + plugin = { + "name": "test", + "description": plugin_desc if plugin_desc is not None else f"pipeline: {len(names)} skills, 9 modes", + } + (root / ".claude-plugin" / "plugin.json").write_text( + json.dumps(plugin, indent=2), encoding="utf-8" + ) + + +def _write_symlinks(root: Path, names: tuple[str, ...]) -> None: + skills = root / "skills" + skills.mkdir(exist_ok=True) + for n in names: + os.symlink(f"../{n}", skills / n) + + +def _write_registry(root: Path, text: str) -> None: + (root / "MODE_REGISTRY.md").write_text(text, encoding="utf-8") + + +def _build_root(root: Path, names: tuple[str, ...] = NAMES) -> None: + for n in names: + write_skill(root, n, f"name: {n}\ndescription: t\nmetadata:\n version: '1'\n") + _write_symlinks(root, names) + _write_table(root, names) + _write_manifests(root, names) + _write_registry(root, f"# Modes\n\n**9 modes** across {len(names)} skills.\n") + + +def _violations(root: Path) -> list[str]: + return run_all_checks(root) + + +def _assert_single(violations: list[str], needle: str) -> None: + assert len(violations) == 1, violations + assert needle in violations[0], violations + + +# ---- baseline --------------------------------------------------------------- + +def test_consistent_root_passes(tmp_path: Path) -> None: + _build_root(tmp_path) + assert _violations(tmp_path) == [] + + +def test_cli_exit_codes(tmp_path: Path) -> None: + _build_root(tmp_path) + ok = run_skill_linter(SCRIPT, tmp_path) + assert ok.returncode == 0, ok.stdout + ok.stderr + assert "OK:" in ok.stdout + (tmp_path / "skills" / NAMES[0]).unlink() + bad = run_skill_linter(SCRIPT, tmp_path) + assert bad.returncode == 1 + assert "ERROR:" in bad.stdout + assert "1 violation(s) found." in bad.stderr + + +def test_empty_root_is_a_violation(tmp_path: Path) -> None: + _assert_single(_violations(tmp_path), "no top-level /SKILL.md found") + + +# ---- surface B: skills/ symlinks ------------------------------------------- + +def test_unpackaged_skill_dir_fails(tmp_path: Path) -> None: + """A fifth directory on disk that nothing else knows about (the #807 shape).""" + _build_root(tmp_path) + write_skill(tmp_path, "gamma-skill", "name: gamma-skill\n") + v = _violations(tmp_path) + assert len(v) == 6, v + missing = [line for line in v if "'gamma-skill' exists on disk" in line] + assert len(missing) == 3, v + assert any("skills/ symlinks" in line for line in missing) + assert any("Skills Overview table" in line for line in missing) + assert any("marketplace.json plugins[].skills" in line for line in missing) + # the "2 skills" claims are now stale on every count surface + counts = [line for line in v if "claims '2 skills' but 3" in line] + assert len(counts) == 3, v # marketplace, plugin.json, MODE_REGISTRY.md + + +def test_missing_symlink_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / "skills" / NAMES[1]).unlink() + _assert_single(_violations(tmp_path), f"'{NAMES[1]}' exists on disk") + + +def test_extra_symlink_without_dir_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + os.symlink("../ghost", tmp_path / "skills" / "ghost") + v = _violations(tmp_path) + assert len(v) == 2, v + assert any("dangling symlink" in line for line in v) + assert any("lists skill 'ghost' but no top-level" in line for line in v) + + +def test_symlink_to_wrong_target_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + link = tmp_path / "skills" / NAMES[0] + link.unlink() + os.symlink(f"../{NAMES[1]}", link) + _assert_single(_violations(tmp_path), "symlink resolves to") + + +def test_real_directory_instead_of_symlink_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + link = tmp_path / "skills" / NAMES[0] + link.unlink() + link.mkdir() + _assert_single(_violations(tmp_path), "must be a symlink") + + +def test_missing_skills_dir_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + for n in NAMES: + (tmp_path / "skills" / n).unlink() + (tmp_path / "skills").rmdir() + v = _violations(tmp_path) + assert any("directory is missing" in line for line in v) + assert sum("not listed in skills/ symlinks" in line for line in v) == len(NAMES) + + +# ---- surface C: CLAUDE.md table -------------------------------------------- + +def test_missing_table_row_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_table(tmp_path, NAMES[:1]) + _assert_single(_violations(tmp_path), f"'{NAMES[1]}' exists on disk") + + +def test_extra_table_row_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_table(tmp_path, NAMES + ("stale-skill",)) + _assert_single(_violations(tmp_path), "lists skill 'stale-skill' but no top-level") + + +def test_rows_outside_section_are_ignored(tmp_path: Path) -> None: + """The fixture table carries a `not-a-skill` row under a later heading.""" + _build_root(tmp_path) + assert _violations(tmp_path) == [] + + +def _replace_alpha_row(root: Path, row: str) -> None: + md = root / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8") + md.write_text(text.replace("| `alpha-skill` v1.0.0 | purpose | full |", row), encoding="utf-8") + + +@pytest.mark.parametrize("row", [ + "| `alpha-skill` | no version at all | full |", + "| `alpha-skill` 1.0.0 | version without v | full |", +]) +def test_row_without_version_token_fails(tmp_path: Path, row: str) -> None: + """A row the version lint would skip is still counted as listed AND reported.""" + _build_root(tmp_path) + _replace_alpha_row(tmp_path, row) + _assert_single(_violations(tmp_path), "row for 'alpha-skill' lacks a 'vX.Y.Z' token") + + +@pytest.mark.parametrize("name", ["Ghost-Skill", "ghost_skill", "ghost skill", "ghost.skill"]) +def test_non_canonical_row_name_is_reported(tmp_path: Path, name: str) -> None: + """A stale advertised row with a non-canonical name must not be invisible.""" + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8").replace( + "| `beta-skill` v1.0.0 | purpose | full |", + f"| `beta-skill` v1.0.0 | purpose | full |\n| `{name}` v1.0.0 | stale | x |", + ) + md.write_text(text, encoding="utf-8") + _assert_single(_violations(tmp_path), f"row names {name!r}, which is not a canonical") + + +@pytest.mark.parametrize("row", [ + "| ghost-skill v1.0.0 | no backticks | x |", + "| **ghost-skill** v1.0.0 | bold instead | x |", + "| | empty first cell | x |", +]) +def test_data_row_without_backticked_name_is_reported(tmp_path: Path, row: str) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8").replace( + "| `beta-skill` v1.0.0 | purpose | full |\n", + f"| `beta-skill` v1.0.0 | purpose | full |\n{row}\n", 1) + md.write_text(text, encoding="utf-8") + _assert_single(_violations(tmp_path), "has no backticked skill name in its first cell") + + +@pytest.mark.parametrize("separator", ["|-------|---------|-----------|", "|:--|:-:|--:|", "| --- | --- | --- |"]) +def test_header_and_separator_rows_are_not_data_rows(tmp_path: Path, separator: str) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + md.write_text(md.read_text(encoding="utf-8").replace("|-------|---------|-----------|", separator), encoding="utf-8") + assert _violations(tmp_path) == [] + + +@pytest.mark.parametrize("mutation", ["drop_separator", "drop_header_and_separator"]) +def test_table_without_separator_is_not_a_table(tmp_path: Path, mutation: str) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8").replace("|-------|---------|-----------|\n", "", 1) + if mutation == "drop_header_and_separator": + text = text.replace("| Skill | Purpose | Key Modes |\n", "", 1) + md.write_text(text, encoding="utf-8") + v = _violations(tmp_path) + assert any("is not followed by a GFM table" in line for line in v), v + + +@pytest.mark.parametrize("separator", ["|---|", "|---|---|", "|---|---|---|---|"]) +def test_separator_width_must_match_header(tmp_path: Path, separator: str) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + md.write_text(md.read_text(encoding="utf-8").replace("|-------|---------|-----------|", separator), encoding="utf-8") + v = _violations(tmp_path) + assert any("GFM requires them equal" in line for line in v), v + + +def test_escaped_pipe_in_header_is_one_cell(tmp_path: Path) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + md.write_text(md.read_text(encoding="utf-8").replace("| Skill | Purpose | Key Modes |", "| Skill | Purpose \\| Role | Key Modes |"), encoding="utf-8") + assert _violations(tmp_path) == [] + + +def test_non_semver_version_token_is_left_to_the_version_lint(tmp_path: Path) -> None: + """`vNOPE` satisfies the shared FULL grammar; check_version_consistency.py + rejects it as non-semver, so parity stays silent by design.""" + _build_root(tmp_path) + _replace_alpha_row(tmp_path, "| `alpha-skill` vNOPE | placeholder | full |") + assert _violations(tmp_path) == [] + + +def test_missing_section_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / ".claude" / "CLAUDE.md").write_text("# nothing\n", encoding="utf-8") + v = _violations(tmp_path) + assert any("H2 is missing" in line for line in v) + + +def test_demoted_heading_is_not_the_section(tmp_path: Path) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + md.write_text(md.read_text(encoding="utf-8").replace("## Skills Overview", "### Skills Overview"), encoding="utf-8") + v = _violations(tmp_path) + assert any("H2 is missing" in line for line in v), v + + +def test_fenced_heading_copy_does_not_bind(tmp_path: Path) -> None: + """A fenced example carrying the heading + a ghost row sits BEFORE the real + section; parity must bind to the real H2 and ignore the ghost.""" + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + fenced = "```markdown\n## Skills Overview\n\n| `ghost` v9.9.9 | x | y |\n```\n\n" + md.write_text(md.read_text(encoding="utf-8").replace("## Skills Overview", fenced + "## Skills Overview", 1), encoding="utf-8") + assert _violations(tmp_path) == [] + + +@pytest.mark.parametrize("trailer", [ + "\n| Other | Table |\n|---|---|\n| `ghost` v1.0.0 | second table |\n", + "\n```\n| `ghost` v1.0.0 | fenced sample |\n```\n", + "\nProse mentioning `ghost` v1.0.0 | in a pipe-free line.\n", +]) +def test_only_the_first_table_after_the_heading_counts(tmp_path: Path, trailer: str) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8").replace("| `beta-skill` v1.0.0 | purpose | full |\n", + "| `beta-skill` v1.0.0 | purpose | full |\n" + trailer, 1) + md.write_text(text, encoding="utf-8") + assert _violations(tmp_path) == [] + + +def test_skill_only_in_a_second_table_is_not_listed(tmp_path: Path) -> None: + _build_root(tmp_path) + md = tmp_path / ".claude" / "CLAUDE.md" + text = md.read_text(encoding="utf-8").replace( + "| `beta-skill` v1.0.0 | purpose | full |\n", + "\n| Other | Table |\n|---|---|\n| `beta-skill` v1.0.0 | moved here |\n", 1) + md.write_text(text, encoding="utf-8") + _assert_single(_violations(tmp_path), "'beta-skill' exists on disk") + + +def test_missing_claude_md_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / ".claude" / "CLAUDE.md").unlink() + v = _violations(tmp_path) + assert any("CLAUDE.md: file is missing" in line for line in v) + + +# ---- surface D: marketplace.json ------------------------------------------- + +def test_missing_manifest_entry_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_entries=[f"./{NAMES[0]}"]) + _assert_single(_violations(tmp_path), f"'{NAMES[1]}' exists on disk") + + +def test_extra_manifest_entry_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_entries=[f"./{n}" for n in NAMES] + ["./stale"]) + _assert_single(_violations(tmp_path), "lists skill 'stale' but no top-level") + + +@pytest.mark.parametrize("bad", ["alpha-skill", "skills/alpha-skill", "./Alpha", "./a/b", 7, "./alpha-skill\n", "./alpha-skill "]) +def test_malformed_manifest_entry_fails(tmp_path: Path, bad) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_entries=[bad, f"./{NAMES[1]}"]) + v = _violations(tmp_path) + assert any("must be './'" in line for line in v), v + # the malformed entry does not count as listing alpha-skill + assert any(f"'{NAMES[0]}' exists on disk" in line for line in v), v + + +def test_invalid_manifest_json_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / ".claude-plugin" / "marketplace.json").write_text("{", encoding="utf-8") + v = _violations(tmp_path) + assert any("invalid JSON" in line for line in v) + + +def test_missing_marketplace_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / ".claude-plugin" / "marketplace.json").unlink() + v = _violations(tmp_path) + assert any("marketplace.json: file is missing" in line for line in v) + + +# ---- count claims ----------------------------------------------------------- + +def test_stale_marketplace_count_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_desc="4 skills + 27 modes") + _assert_single(_violations(tmp_path), "marketplace.json description: claims '4 skills'") + + +def test_stale_marketplace_top_level_count_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_top_desc="A marketplace of 3 skills") + _assert_single(_violations(tmp_path), "marketplace.json description: claims '3 skills'") + + +def test_stale_plugin_count_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, plugin_desc="4 skills, 27 modes") + _assert_single(_violations(tmp_path), "plugin.json description: claims '4 skills'") + + +@pytest.mark.parametrize("desc", ["4 Skills + 27 modes", "4 SKILLS"]) +def test_count_claim_is_case_insensitive(tmp_path: Path, desc: str) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, plugin_desc=desc) + _assert_single(_violations(tmp_path), "plugin.json description: claims") + + +def test_description_without_count_makes_no_claim(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_manifests(tmp_path, NAMES, market_desc="research skills for Claude Code", plugin_desc="40 skillsets") + assert _violations(tmp_path) == [] + + +def test_stale_mode_registry_count_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + _write_registry(tmp_path, "**27 modes** across 4 skills.\n") + _assert_single(_violations(tmp_path), "MODE_REGISTRY.md: claims '4 skills'") + + +def test_missing_mode_registry_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / "MODE_REGISTRY.md").unlink() + _assert_single(_violations(tmp_path), "MODE_REGISTRY.md: file is missing") + + +def test_missing_plugin_json_fails(tmp_path: Path) -> None: + _build_root(tmp_path) + (tmp_path / ".claude-plugin" / "plugin.json").unlink() + _assert_single(_violations(tmp_path), "plugin.json: file is missing") + + +# ---- real tree -------------------------------------------------------------- + +def test_real_tree_passes() -> None: + assert run_all_checks(REPO_ROOT) == [] diff --git a/scripts/test_check_spec_consistency.py b/scripts/test_check_spec_consistency.py index 07aa2c13..c01983c0 100644 --- a/scripts/test_check_spec_consistency.py +++ b/scripts/test_check_spec_consistency.py @@ -787,6 +787,18 @@ class TestSkillVersionTableConsistency(unittest.TestCase): csc.ERRORS.clear() csc.ERRORS.extend(self._orig_errors) + def test_skill_paths_follow_the_active_root(self) -> None: + """#809: paths derive from ROOT at call time, so a fixture tree with a + different skill set is policed on ITS skills, never the checkout's.""" + with TemporaryDirectory() as tmp: + root = Path(tmp) + csc.ROOT = root + (root / "only-skill").mkdir() + (root / "only-skill" / "SKILL.md").write_text("---\nname: only-skill\n---\n", encoding="utf-8") + self.assertEqual(csc._skill_version_paths(), ("only-skill/SKILL.md",)) + csc.check_skill_version_blocks() + self.assertTrue(all(e.startswith("only-skill/SKILL.md:") for e in csc.ERRORS), csc.ERRORS) + def test_all_four_aligned_passes(self) -> None: """All four SKILL.md with frontmatter matching their table → no errors.""" with TemporaryDirectory() as tmp: