From 5bfa3a01ae099161e957e436c38aa51e8fa852ae Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 01:08:49 +0300 Subject: [PATCH 1/6] feat(schemas): add SKILL_REQUIREMENTS_SCHEMA for skill runtime deps (ST-001) Define the draft-2020-12 sub-block schema validating a skill's optional requires-env/requires-pip/requires-cmd/requires-skills declarations, with additionalProperties:false to catch typos. Expose SKILL_REQUIREMENTS_KEYS as the single derivable authority so the upcoming consistency test and pre-install check derive the field list rather than hardcoding it. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mapify_cli/schemas.py | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/mapify_cli/schemas.py b/src/mapify_cli/schemas.py index 6ff078f..cbf985d 100644 --- a/src/mapify_cli/schemas.py +++ b/src/mapify_cli/schemas.py @@ -648,6 +648,49 @@ def load_and_validate( } +SKILL_REQUIREMENTS_SCHEMA = { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://mapframework.dev/schemas/skill-requirements.json", + "title": "MAP Skill Requirements", + "description": ( + "Runtime-dependency sub-block for a MAP skill entry in skill-rules.json. " + "All four requires-* fields are optional; omit any that are not needed." + ), + "type": "object", + "properties": { + "requires-env": { + "type": "array", + "items": {"type": "string"}, + "description": "Environment variable names that must be set at pre-install check time.", + }, + "requires-pip": { + "type": "array", + "items": {"type": "string"}, + "description": ( + "Python packages that must be importable. " + "Values are Python IMPORT names (e.g. 'yaml', not 'PyYAML')." + ), + }, + "requires-cmd": { + "type": "array", + "items": {"type": "string"}, + "description": "CLI commands that must be available on PATH.", + }, + "requires-skills": { + "type": "array", + "items": {"type": "string"}, + "description": "Other skill names this skill depends on.", + }, + }, + "required": [], + "additionalProperties": False, +} + +# Single authority for the four requires-* field names; consumers DERIVE from this +# rather than hardcoding the list (see architecture-patterns: Single-Source Schema Dict). +SKILL_REQUIREMENTS_KEYS = tuple(SKILL_REQUIREMENTS_SCHEMA["properties"]) + + ARTIFACT_STAGE_SCHEMA = { "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://mapframework.dev/schemas/artifact-stage.json", From 56f6cedfcacfb503b81af24418b1327bf9eb309f Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 01:15:09 +0300 Subject: [PATCH 2/6] feat(skills): declare map-state requires-cmd:[git] in skill-rules (ST-002) Add the requires-cmd declaration to the single-source skill-rules.json.jinja for map-state (its scripts invoke git); render propagates to both generated trees. The other 13 skills ship no scripts and declare no requirements. make check-render green; tests/test_skills.py (223) pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/skill-rules.json | 1 + src/mapify_cli/templates/skills/skill-rules.json | 1 + src/mapify_cli/templates_src/skills/skill-rules.json.jinja | 1 + 3 files changed, 3 insertions(+) diff --git a/.claude/skills/skill-rules.json b/.claude/skills/skill-rules.json index 473f56c..2d733e5 100644 --- a/.claude/skills/skill-rules.json +++ b/.claude/skills/skill-rules.json @@ -12,6 +12,7 @@ "hooks", "branch-scoped .map artifacts" ], + "requires-cmd": ["git"], "promptTriggers": { "keywords": [ "create plan", diff --git a/src/mapify_cli/templates/skills/skill-rules.json b/src/mapify_cli/templates/skills/skill-rules.json index 473f56c..2d733e5 100644 --- a/src/mapify_cli/templates/skills/skill-rules.json +++ b/src/mapify_cli/templates/skills/skill-rules.json @@ -12,6 +12,7 @@ "hooks", "branch-scoped .map artifacts" ], + "requires-cmd": ["git"], "promptTriggers": { "keywords": [ "create plan", diff --git a/src/mapify_cli/templates_src/skills/skill-rules.json.jinja b/src/mapify_cli/templates_src/skills/skill-rules.json.jinja index 473f56c..2d733e5 100644 --- a/src/mapify_cli/templates_src/skills/skill-rules.json.jinja +++ b/src/mapify_cli/templates_src/skills/skill-rules.json.jinja @@ -12,6 +12,7 @@ "hooks", "branch-scoped .map artifacts" ], + "requires-cmd": ["git"], "promptTriggers": { "keywords": [ "create plan", From c14656a8f05a1a5ca9f7f7097f3590035e90c20e Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 01:35:40 +0300 Subject: [PATCH 3/6] test(skills): add skill dependency consistency test (ST-003) New tests/test_skills_consistency.py scans each shipped skill's scripts (*.sh via a conservative POSIX-builtin-aware scanner, *.py via AST) and asserts the declared requires-* (skill-rules.json) is a superset of the scanner-detected deps, validates each requires-* sub-block against SKILL_REQUIREMENTS_SCHEMA, and rejects dangling requires-skills targets. Green on all 14 skills (map-state: git detected + declared). Includes a teeth self-test proving under-declaration fails, plus scanner-edge unit tests (getenv-default, comments, builtins, awk programs, heredoc bodies, apostrophes). SCANNABLE_KEYS derives from SKILL_REQUIREMENTS_KEYS (INV-2). Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_skills_consistency.py | 885 +++++++++++++++++++++++++++++++ 1 file changed, 885 insertions(+) create mode 100644 tests/test_skills_consistency.py diff --git a/tests/test_skills_consistency.py b/tests/test_skills_consistency.py new file mode 100644 index 0000000..f322e6e --- /dev/null +++ b/tests/test_skills_consistency.py @@ -0,0 +1,885 @@ +""" +Consistency tests: declared requires-* in skill-rules.json ⊇ scanner-detected deps. + +VC1: per-skill, declared requires-* ⊇ scanner-detected; under-declaration FAILS. +VC2: GREEN on all 14 existing skills (map-state git detected + declared; others empty). +VC3: each requires-* sub-block validates vs SKILL_REQUIREMENTS_SCHEMA; no dangling + requires-skills targets. +VC4: reuses skill_rules fixture; scanner ignores os.getenv(x, default) and comments. +""" + +from __future__ import annotations + +import ast +import copy +import json +import re +from pathlib import Path +from typing import Any + +import pytest + +from mapify_cli.schemas import ( + SKILL_REQUIREMENTS_KEYS, + SKILL_REQUIREMENTS_SCHEMA, + validate_artifact, +) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +# POSIX/shell built-ins that are safe to ignore during command scanning. +# Deliberately broad and conservative to avoid false positives. +_SHELL_BUILTINS: frozenset[str] = frozenset( + { + # flow control keywords + "if", + "else", + "elif", + "fi", + "then", + "while", + "for", + "do", + "done", + "case", + "esac", + "function", + # builtins + "echo", + "printf", + "read", + "export", + "local", + "set", + "unset", + "shift", + "eval", + "exec", + "source", + ".", + "exit", + "return", + "true", + "false", + "cd", + "pwd", + "test", + "[", + "[[", + "]]", + "]", + # common POSIX utilities + "grep", + "sed", + "awk", + "mkdir", + "cp", + "mv", + "rm", + "touch", + "ls", + "cat", + "head", + "tail", + "wc", + "sort", + "uniq", + "tr", + "cut", + "xargs", + "find", + "env", + "date", + "sleep", + "dirname", + "basename", + "tee", + # bash-specific + "declare", + "typeset", + "readonly", + "hash", + "type", + "command", + "builtin", + "getopts", + "trap", + "wait", + "jobs", + "kill", + "disown", + "mapfile", + "readarray", + } +) + +# Standard-library top-level module names (Python 3.11). +# Only third-party imports are flagged for requires-pip. +_STDLIB_MODULES: frozenset[str] = frozenset( + { + "__future__", + "_thread", + "abc", + "argparse", + "ast", + "asyncio", + "base64", + "binascii", + "builtins", + "calendar", + "cmath", + "cmd", + "code", + "codecs", + "collections", + "colorsys", + "compileall", + "concurrent", + "configparser", + "contextlib", + "copy", + "copyreg", + "csv", + "ctypes", + "curses", + "dataclasses", + "datetime", + "dbm", + "decimal", + "difflib", + "dis", + "doctest", + "email", + "encodings", + "enum", + "errno", + "faulthandler", + "fcntl", + "filecmp", + "fileinput", + "fnmatch", + "fractions", + "ftplib", + "functools", + "gc", + "getopt", + "getpass", + "gettext", + "glob", + "gzip", + "hashlib", + "heapq", + "hmac", + "html", + "http", + "idlelib", + "imaplib", + "importlib", + "inspect", + "io", + "ipaddress", + "itertools", + "json", + "keyword", + "linecache", + "locale", + "logging", + "lzma", + "mailbox", + "marshal", + "math", + "mimetypes", + "mmap", + "modulefinder", + "multiprocessing", + "netrc", + "numbers", + "operator", + "optparse", + "os", + "pathlib", + "pdb", + "pkgutil", + "platform", + "plistlib", + "poplib", + "posix", + "posixpath", + "pprint", + "profile", + "pstats", + "pty", + "py_compile", + "pyclbr", + "pydoc", + "queue", + "quopri", + "random", + "re", + "readline", + "reprlib", + "resource", + "rlcompleter", + "runpy", + "sched", + "secrets", + "select", + "selectors", + "shelve", + "shlex", + "shutil", + "signal", + "site", + "smtplib", + "socket", + "socketserver", + "sqlite3", + "ssl", + "stat", + "statistics", + "string", + "stringprep", + "struct", + "subprocess", + "sys", + "sysconfig", + "syslog", + "tabnanny", + "tarfile", + "telnetlib", + "tempfile", + "termios", + "test", + "textwrap", + "threading", + "time", + "timeit", + "tkinter", + "token", + "tokenize", + "tomllib", + "trace", + "traceback", + "tracemalloc", + "tty", + "turtle", + "types", + "typing", + "unicodedata", + "unittest", + "urllib", + "uuid", + "venv", + "warnings", + "wave", + "weakref", + "webbrowser", + "wsgiref", + "xml", + "xmlrpc", + "zipapp", + "zipfile", + "zipimport", + "zlib", + "zoneinfo", + } +) + +# ALL_CAPS tokens are shell variable references (e.g. BRANCH, PLAN_FILE), not commands. +_ALL_CAPS_RE = re.compile(r"^[A-Z_][A-Z0-9_]*$") + +# Scannable requires-* keys: keys whose values can be detected by static analysis. +# requires-skills is intentionally excluded — no automated scanner can detect +# cross-skill dependencies from source code alone. +SCANNABLE_KEYS: tuple[str, ...] = tuple( + k for k in SKILL_REQUIREMENTS_KEYS if k != "requires-skills" +) + +# Command-position regex: first word after start-of-line, ;, |, &&, ||, (, { +_CMD_POSITION_RE = re.compile( + r"(?:^|[;|{(&]|&&|\|\|)\s*([A-Za-z0-9_./-]+)", + re.MULTILINE, +) + +# --------------------------------------------------------------------------- +# Project root + parametrize list (evaluated at collection time) +# --------------------------------------------------------------------------- + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent + + +def _skill_names_from_dir() -> list[str]: + """Collect skill folder names from .claude/skills/ at import time.""" + skills_path = _PROJECT_ROOT / ".claude" / "skills" + if not skills_path.exists(): + return [] + return sorted( + d.name + for d in skills_path.iterdir() + if d.is_dir() and not d.name.startswith(".") + ) + + +_ALL_SKILL_NAMES: list[str] = _skill_names_from_dir() + +# --------------------------------------------------------------------------- +# Module-scoped fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def skills_dir() -> Path: + d = _PROJECT_ROOT / ".claude" / "skills" + if not d.exists(): + pytest.skip(".claude/skills/ directory doesn't exist") + return d + + +@pytest.fixture(scope="module") +def skill_rules(skills_dir: Path) -> dict[str, Any]: + rules_file = skills_dir / "skill-rules.json" + if not rules_file.exists(): + pytest.skip("skill-rules.json doesn't exist") + return json.loads(rules_file.read_text()) + + +@pytest.fixture(scope="module") +def skill_names(skill_rules: dict[str, Any]) -> list[str]: + return list(skill_rules.get("skills", {}).keys()) + + +# --------------------------------------------------------------------------- +# Shell command scanner +# --------------------------------------------------------------------------- + + +def _scan_sh_commands(path: Path) -> set[str]: + """Return non-builtin command names invoked in a shell script. + + Conservative scanner that avoids false positives: + - Skips pure comment lines (first non-space char is '#'). + - Strips inline ' # ...' tail comments. + - Strips double-quoted strings FIRST (may contain embedded apostrophes). + - Tracks multi-line single-quoted blocks (awk/sed programs) via a state + machine; lines inside such blocks are skipped entirely. + - Strips single-line single-quoted strings. + - Filters ALL_CAPS tokens (shell variable references, not commands). + - Filters locally defined shell function names. + - Filters _SHELL_BUILTINS, variable assignments, $ expansions, and paths. + """ + detected: set[str] = set() + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + return detected + + # Collect locally defined function names to exclude their call-sites. + defined_functions: set[str] = set() + for m in re.finditer(r"^([A-Za-z_][A-Za-z0-9_]*)\s*\(\s*\)", text, re.MULTILINE): + defined_functions.add(m.group(1)) + for m in re.finditer( + r"^\s*function\s+([A-Za-z_][A-Za-z0-9_]*)", text, re.MULTILINE + ): + defined_functions.add(m.group(1)) + + in_sq_block = False # True while inside a multi-line single-quoted string + in_heredoc = False # True while inside a here-document body + heredoc_term = "" # Terminator word (bare, unquoted) to watch for + heredoc_indented = False # True for <<- variant (tab-stripped terminator) + + for line in text.splitlines(): + stripped = line.strip() + + if in_heredoc: + # Inside a here-document body — skip scanning entirely. + # For <<- the terminator may be tab-indented; strip tabs before compare. + compare = line.lstrip("\t") if heredoc_indented else line + if compare.rstrip("\n") == heredoc_term: + in_heredoc = False + continue + + if in_sq_block: + # Inside a multi-line single-quoted block — skip scanning. + # Strip double-quoted content before counting bare ' to avoid + # embedded apostrophes in "won't" style strings confusing the count. + dq_clean = re.sub(r'"[^"]*"', '""', line) + bare_sq = len(re.findall(r"(? None: + """Inspect a Call AST node for os.getenv (env) and subprocess calls (cmd).""" + func = node.func + + # os.getenv('KEY') → requires-env, but ONLY when there is no default. + # os.getenv('KEY', 'fallback') is intentionally ignored. + if ( + isinstance(func, ast.Attribute) + and isinstance(func.value, ast.Name) + and func.value.id == "os" + and func.attr == "getenv" + ): + args = node.args + has_default = len(args) >= 2 or any( + kw.arg == "default" for kw in node.keywords + ) + if not has_default and args: + key_node = args[0] + if isinstance(key_node, ast.Constant) and isinstance( + key_node.value, str + ): + result["requires-env"].add(key_node.value) + return + + _SUBPROCESS_ATTRS = frozenset( + {"run", "Popen", "call", "check_output", "check_call"} + ) + _OS_PROC_ATTRS = frozenset({"system", "popen"}) + + is_subprocess_call = ( + isinstance(func, ast.Attribute) + and isinstance(func.value, ast.Name) + and func.value.id == "subprocess" + and func.attr in _SUBPROCESS_ATTRS + ) + is_os_proc = ( + isinstance(func, ast.Attribute) + and isinstance(func.value, ast.Name) + and func.value.id == "os" + and func.attr in _OS_PROC_ATTRS + ) + + if not (is_subprocess_call or is_os_proc): + return + + if not node.args: + return + first_arg = node.args[0] + + if isinstance(first_arg, ast.Constant) and isinstance(first_arg.value, str): + # String form: subprocess.run("git status") + cmd_token = first_arg.value.split()[0] if first_arg.value.strip() else None + if cmd_token: + result["requires-cmd"].add(cmd_token) + elif isinstance(first_arg, ast.List) and first_arg.elts: + # List form: subprocess.run(["git", "status"]) + first_elt = first_arg.elts[0] + if isinstance(first_elt, ast.Constant) and isinstance( + first_elt.value, str + ): + result["requires-cmd"].add(first_elt.value) + + +def _scan_py_deps(path: Path) -> dict[str, set[str]]: + """Scan a Python file for requires-pip / requires-env / requires-cmd. + + requires-pip: top-level imports whose root module is NOT in _STDLIB_MODULES. + requires-env: os.environ['KEY'] or os.getenv('KEY') with NO default. + requires-cmd: subprocess.* / os.system / os.popen with a literal command. + """ + result: dict[str, set[str]] = { + "requires-pip": set(), + "requires-env": set(), + "requires-cmd": set(), + } + try: + source = path.read_text(encoding="utf-8", errors="replace") + tree = ast.parse(source, filename=str(path)) + except (OSError, SyntaxError): + return result + + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + root = alias.name.split(".")[0] + if root not in _STDLIB_MODULES: + result["requires-pip"].add(root) + + elif isinstance(node, ast.ImportFrom): + if node.module: + root = node.module.split(".")[0] + if root and root not in _STDLIB_MODULES: + result["requires-pip"].add(root) + + # os.environ['KEY'] → requires-env + elif isinstance(node, ast.Subscript): + if ( + isinstance(node.value, ast.Attribute) + and isinstance(node.value.value, ast.Name) + and node.value.value.id == "os" + and node.value.attr == "environ" + ): + key_node = node.slice + if isinstance(key_node, ast.Constant) and isinstance( + key_node.value, str + ): + result["requires-env"].add(key_node.value) + + elif isinstance(node, ast.Call): + _check_call_node(node, result) + + return result + + +# --------------------------------------------------------------------------- +# Aggregate scanner +# --------------------------------------------------------------------------- + + +def detect_skill_deps(skill_dir: Path) -> dict[str, set[str]]: + """Scan all scripts under /scripts/ and return detected deps.""" + detected: dict[str, set[str]] = {k: set() for k in SCANNABLE_KEYS} + scripts_dir = skill_dir / "scripts" + if not scripts_dir.is_dir(): + return detected + + for script in sorted(scripts_dir.iterdir()): + if not script.is_file(): + continue + if script.suffix == ".sh": + detected["requires-cmd"].update(_scan_sh_commands(script)) + elif script.suffix == ".py": + py_deps = _scan_py_deps(script) + for key in SCANNABLE_KEYS: + detected[key].update(py_deps[key]) + + return detected + + +# --------------------------------------------------------------------------- +# Discovery guard — non-vacuous sentinel (VC2) +# --------------------------------------------------------------------------- + + +def test_skill_discovery_non_empty(skill_names: list[str]) -> None: + """Guard: skill-rules.json must list exactly 14 skills (prevents vacuous pass).""" + assert len(skill_names) == 14, ( + f"Expected 14 skills in skill-rules.json, found {len(skill_names)}: " + f"{sorted(skill_names)}" + ) + + +# --------------------------------------------------------------------------- +# VC1 — declared requires-* ⊇ scanner-detected (parametrized per skill) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("skill_name", _ALL_SKILL_NAMES) +def test_declared_superset_of_detected( + skill_name: str, + skill_rules: dict[str, Any], + skills_dir: Path, +) -> None: + """VC1: declared requires-* ⊇ scanner-detected; under-declaration FAILS.""" + skill_entry: dict[str, Any] = skill_rules.get("skills", {}).get(skill_name, {}) + skill_dir = skills_dir / skill_name + detected = detect_skill_deps(skill_dir) + + undeclared: list[str] = [] + + for key in SCANNABLE_KEYS: + detected_vals = detected[key] + if not detected_vals: + continue + declared_vals: set[str] = set(skill_entry.get(key) or []) + missing = detected_vals - declared_vals + if missing: + undeclared.append( + f" {key}: detected {sorted(missing)} but not declared" + ) + + assert not undeclared, ( + f"Skill '{skill_name}' has under-declared requirements:\n" + + "\n".join(undeclared) + + "\n\nCurrent declared entry:\n" + + json.dumps( + {k: skill_entry.get(k) for k in SKILL_REQUIREMENTS_KEYS}, indent=2 + ) + ) + + +# --------------------------------------------------------------------------- +# VC3a — requires-* sub-block validates vs SKILL_REQUIREMENTS_SCHEMA +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("skill_name", _ALL_SKILL_NAMES) +def test_requires_block_schema_valid( + skill_name: str, + skill_rules: dict[str, Any], +) -> None: + """VC3: Each requires-* sub-block validates against SKILL_REQUIREMENTS_SCHEMA.""" + skill_entry: dict[str, Any] = skill_rules.get("skills", {}).get(skill_name, {}) + + requires_block = { + k: skill_entry[k] for k in SKILL_REQUIREMENTS_KEYS if k in skill_entry + } + + is_valid, errors = validate_artifact(requires_block, SKILL_REQUIREMENTS_SCHEMA) + assert is_valid, ( + f"Skill '{skill_name}' requires-* sub-block is schema-invalid:\n" + + "\n".join(f" - {e}" for e in errors) + + f"\n\nSub-block: {json.dumps(requires_block, indent=2)}" + ) + + +# --------------------------------------------------------------------------- +# VC3b — no dangling requires-skills references +# --------------------------------------------------------------------------- + + +def test_no_dangling_requires_skills(skill_rules: dict[str, Any]) -> None: + """VC3: Every requires-skills target must be a real key in the catalog.""" + catalog: dict[str, Any] = skill_rules.get("skills", {}) + catalog_keys = set(catalog.keys()) + dangling: list[str] = [] + + for skill_name, entry in catalog.items(): + targets: list[str] = entry.get("requires-skills") or [] + for target in targets: + if target not in catalog_keys: + dangling.append( + f" '{skill_name}' requires-skills -> '{target}' (not in catalog)" + ) + + assert not dangling, ( + "Dangling requires-skills references found:\n" + "\n".join(dangling) + ) + + +# --------------------------------------------------------------------------- +# VC4 — scanner unit tests +# --------------------------------------------------------------------------- + + +def test_scanner_ignores_getenv_with_default(tmp_path: Path) -> None: + """VC4: os.getenv('KEY', 'default') must NOT be flagged as a required env var.""" + script = tmp_path / "test_getenv.py" + script.write_text( + "import os\n" + "x = os.getenv('WITH_DEFAULT', 'fallback') # must be ignored\n" + "y = os.getenv('NO_DEFAULT') # must be detected\n" + "z = os.environ['DIRECT_KEY'] # must be detected\n", + encoding="utf-8", + ) + deps = _scan_py_deps(script) + assert "WITH_DEFAULT" not in deps["requires-env"], ( + "getenv with default must be ignored as a required env var" + ) + assert "NO_DEFAULT" in deps["requires-env"], ( + "getenv without default must be detected as a required env var" + ) + assert "DIRECT_KEY" in deps["requires-env"], ( + "os.environ['KEY'] must be detected as a required env var" + ) + + +def test_scanner_ignores_sh_comments(tmp_path: Path) -> None: + """VC4: Comment lines and inline comments in shell scripts must not yield commands.""" + script = tmp_path / "test_comments.sh" + script.write_text( + "#!/usr/bin/env bash\n" + "# docker run something <-- full comment line, must be ignored\n" + "echo hello # kubectl get pods <-- inline comment, must be ignored\n" + "git status # real command follows\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + assert "docker" not in cmds, "Command in full comment line must be ignored" + assert "kubectl" not in cmds, "Command in inline comment must be ignored" + assert "git" in cmds, "git must still be detected on a real command line" + + +def test_scanner_ignores_posix_builtins(tmp_path: Path) -> None: + """VC4: POSIX builtins must not appear in detected requires-cmd.""" + script = tmp_path / "test_builtins.sh" + script.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + "mkdir -p /tmp/x\n" + "cp src dst\n" + "grep pattern file\n" + "git rev-parse HEAD\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + for builtin in ("set", "mkdir", "cp", "grep"): + assert builtin not in cmds, f"Builtin '{builtin}' must not appear in requires-cmd" + assert "git" in cmds, "git must be detected (not a builtin)" + + +def test_scanner_ignores_multiline_awk_program(tmp_path: Path) -> None: + """VC4: Tokens inside a multi-line awk program must not be flagged as commands.""" + script = tmp_path / "test_awk.sh" + script.write_text( + "#!/usr/bin/env bash\n" + "RESULT=$(\n" + " awk '\n" + " /pattern/ { getline; print; next }\n" + " in_section { print }\n" + " ' \"$INFILE\"\n" + ")\n" + "git log --oneline\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + for awk_token in ("getline", "print", "next", "in_section"): + assert awk_token not in cmds, ( + f"awk token '{awk_token}' inside awk program must not be flagged" + ) + assert "git" in cmds, "git outside awk block must still be detected" + + +def test_scanner_ignores_won_t_do_in_echo(tmp_path: Path) -> None: + """VC4: Apostrophe inside a double-quoted string (won't_do) must not open sq block.""" + script = tmp_path / "test_wontdo.sh" + script.write_text( + "#!/usr/bin/env bash\n" + "echo \"won't_do count: $COUNT\"\n" + "git status\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + assert "git" in cmds, "git must be detected after double-quoted string with apostrophe" + + +def test_scanner_ignores_heredoc_body(tmp_path: Path) -> None: + """VC4: Tokens inside a heredoc body must not be flagged as required commands.""" + script = tmp_path / "test_heredoc.sh" + script.write_text( + "#!/usr/bin/env bash\n" + "cat <<'EOF'\n" + "docker run something\n" + "EOF\n" + "git status\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + assert "docker" not in cmds, ( + "Command inside heredoc body must not be flagged as a required command" + ) + assert "git" in cmds, "git outside the heredoc must still be detected" + + +# --------------------------------------------------------------------------- +# Teeth self-test — under-declaration actually fails (not vacuously always-green) +# --------------------------------------------------------------------------- + + +def test_scanner_teeth_under_declaration_detected( + skill_rules: dict[str, Any], + skills_dir: Path, +) -> None: + """Self-test: emptying map-state requires-cmd triggers under-declaration failure. + + Proves VC1 has actual teeth — the assertion is NOT vacuously always-green. + """ + mutated_rules = copy.deepcopy(skill_rules) + map_state_entry: dict[str, Any] = mutated_rules["skills"]["map-state"] + + original_requires_cmd: list[str] = map_state_entry.get("requires-cmd") or [] + assert original_requires_cmd, ( + "Pre-condition: map-state must have a non-empty requires-cmd in " + "skill-rules.json for this self-test to be meaningful" + ) + + # Simulate under-declaration by clearing the declared list in-memory only. + map_state_entry["requires-cmd"] = [] + + skill_dir = skills_dir / "map-state" + detected = detect_skill_deps(skill_dir) + + undeclared: list[str] = [] + + for key in SCANNABLE_KEYS: + detected_vals = detected[key] + if not detected_vals: + continue + declared_vals: set[str] = set(map_state_entry.get(key) or []) + missing = detected_vals - declared_vals + if missing: + undeclared.append(f"{key}: {sorted(missing)}") + + assert undeclared, ( + "Teeth self-test FAILED: clearing map-state requires-cmd did NOT trigger " + "under-declaration detection. The VC1 assertion may be vacuously green. " + f"Scanner detected: {detected}" + ) + print( + f"\n[teeth-self-test] PASSED — under-declaration correctly detected: {undeclared}" + ) From 89cd0058f52b6a67657aafdaa7394943c350d0d8 Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 01:55:11 +0300 Subject: [PATCH 4/6] feat(install): host-conditional skill skip in create_skill_files (ST-004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate each shipped skill on its declared requires-* before install: skip (and print [skipped: : missing ]) when a required cmd (shutil.which), pip module (find_spec), or env var NAME (os.environ) is absent on the host. requires-skills is warning-only, never a skip. The gate lives inside create_skill_files so both `mapify init` (providers.py) and `upgrade` (__init__.py) are guarded by one site; happy-path install is unchanged. A module-level assertion enforces _REQUIRES_CHECKER covers every blocking key derived from SKILL_REQUIREMENTS_KEYS, so a new schema key fails loudly at import rather than going silently unchecked. Security: env check reads variable NAMES only — never values, never .env files. Also fixes a latent mypy error on SKILL_REQUIREMENTS_KEYS (tuple of a dict subscript) surfaced by `make check`. Adds tests/test_file_copier.py covering missing-dep skip, deps-present identity, upgrade-path guard, and the name-only env security boundary. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mapify_cli/delivery/file_copier.py | 136 ++++++++++++- src/mapify_cli/schemas.py | 4 +- tests/test_file_copier.py | 265 +++++++++++++++++++++++++ 3 files changed, 399 insertions(+), 6 deletions(-) create mode 100644 tests/test_file_copier.py diff --git a/src/mapify_cli/delivery/file_copier.py b/src/mapify_cli/delivery/file_copier.py index 129de88..983314b 100644 --- a/src/mapify_cli/delivery/file_copier.py +++ b/src/mapify_cli/delivery/file_copier.py @@ -2,6 +2,10 @@ from __future__ import annotations +import importlib.util +import json +import os +import shutil from pathlib import Path from typing import List @@ -18,11 +22,84 @@ create_reflector_content, create_documentation_reviewer_content, ) +from mapify_cli.schemas import SKILL_REQUIREMENTS_KEYS _IGNORED_TEMPLATE_NAMES = {"__pycache__", ".DS_Store"} _IGNORED_TEMPLATE_SUFFIXES = {".pyc", ".pyo"} +# Ordered check dispatch for blocking requires-* keys. +# _BLOCKING_REQUIRES_KEYS is derived from SKILL_REQUIREMENTS_KEYS (schema authority). +# The module-level assertion below enforces that _REQUIRES_CHECKER covers EVERY +# blocking key derived from the schema: adding a new blocking key to +# SKILL_REQUIREMENTS_SCHEMA raises AssertionError at import time unless a +# corresponding checker is added here — the invariant is mechanically enforced, +# not just documented. +# requires-skills is warn-only (not a skip), handled separately. +_BLOCKING_REQUIRES_KEYS = { + k for k in SKILL_REQUIREMENTS_KEYS if k != "requires-skills" +} + + +def _check_requires_cmd(name: str) -> bool: + """Return True if CLI command *name* is available on PATH.""" + return shutil.which(name) is not None + + +def _check_requires_pip(name: str) -> bool: + """Return True if Python module *name* is importable.""" + try: + return importlib.util.find_spec(name) is not None + except (ModuleNotFoundError, ValueError): + return False + + +def _check_requires_env(name: str) -> bool: + """Return True if environment variable *name* is set. + + SECURITY: reads variable NAME presence only — never reads the value, + never accesses .env files. + """ + return name in os.environ + + +_REQUIRES_CHECKER = { + "requires-cmd": _check_requires_cmd, + "requires-pip": _check_requires_pip, + "requires-env": _check_requires_env, +} + +# Enforced invariant: every blocking key derived from the schema must have a +# checker entry here. Adding a new blocking key to SKILL_REQUIREMENTS_SCHEMA +# without a matching checker raises AssertionError at import time. +assert _BLOCKING_REQUIRES_KEYS == set(_REQUIRES_CHECKER), ( + f"_REQUIRES_CHECKER is out of sync with SKILL_REQUIREMENTS_KEYS; " + f"missing checkers for: {_BLOCKING_REQUIRES_KEYS - set(_REQUIRES_CHECKER)}" +) + + +def _skill_missing_dependency(requires_block: dict[str, list[str]]) -> tuple[str, str] | None: + """Return (kind, name) of the first missing blocking dependency, or None. + + Checks requires-cmd, requires-pip, requires-env in that order (dict + insertion order — cmd first, then pip, then env — guarantees deterministic + "first missing" reporting). Every key in _REQUIRES_CHECKER is checked; + the module-level assertion guarantees _REQUIRES_CHECKER == _BLOCKING_REQUIRES_KEYS, + so no blocking key can be silently skipped. + requires-skills is not a blocking dep; call site emits a warning instead. + """ + for kind, checker in _REQUIRES_CHECKER.items(): + for dep_name in requires_block.get(kind, []): + if not checker(dep_name): + return (kind.removeprefix("requires-"), dep_name) + return None + + +def _warn_requires_skills(skill_name: str, skill_names: list[str]) -> None: + """Emit a WARNING for requires-skills entries (read-only; never a skip).""" + for dep in skill_names: + print(f"[warning: {skill_name}: requires skill {dep}]") + def _get_version() -> str: """Get current mapify-cli version for metadata injection.""" @@ -155,11 +232,35 @@ def create_command_files( return 0 +def _load_template_skill_catalog(skills_template_dir: Path) -> dict[str, dict[str, object]]: + """Parse the template skill-rules.json and return the skills dict. + + Returns an empty dict on any error (missing file, invalid JSON) so the + caller falls through to unconditional install — defensive, never gate-blocks + due to a corrupt catalog. + """ + catalog_path = skills_template_dir / "skill-rules.json" + try: + raw = catalog_path.read_text(encoding="utf-8") + data = json.loads(raw) + skills = data.get("skills", {}) + if isinstance(skills, dict): + return skills # type: ignore[return-value] + except Exception: # noqa: BLE001 # FileNotFoundError, JSONDecodeError, etc. + pass + return {} + + def create_skill_files(project_path: Path) -> int: """Create MAP skills in .claude/skills/ + Skips any skill whose blocking runtime dependencies (requires-cmd, + requires-pip, requires-env) are not satisfied on the current host. + Prints ``[skipped: : missing ]`` to stdout for each + skipped skill. requires-skills is WARNING-only and never causes a skip. + Returns: - Number of skills installed + Number of skills actually installed (skipped skills not counted). """ skills_dir = project_path / ".claude" / "skills" skills_dir.mkdir(parents=True, exist_ok=True) @@ -173,6 +274,9 @@ def create_skill_files(project_path: Path) -> int: if skills_template_dir.exists(): version = _get_version() + # Parse catalog ONCE, defensively (missing/invalid -> empty dict -> no gate). + skill_catalog = _load_template_skill_catalog(skills_template_dir) + # Top-level skill catalog files (README.md, skill-rules.json). for top_name in ("README.md", "skill-rules.json"): top_src = skills_template_dir / top_name @@ -180,10 +284,32 @@ def create_skill_files(project_path: Path) -> int: _install_managed_file(top_src, skills_dir / top_name, version) # Copy each skill directory, fence-aware per file (watched category). - for skill_template in skills_template_dir.iterdir(): - if skill_template.is_dir() and skill_template.name != "__pycache__": - _install_managed_tree(skill_template, skills_dir / skill_template.name, version) - count += 1 + for skill_template in sorted(skills_template_dir.iterdir()): + if not (skill_template.is_dir() and skill_template.name != "__pycache__"): + continue + + skill_name = skill_template.name + entry = skill_catalog.get(skill_name, {}) + requires_block: dict[str, list[str]] = { + k: v # type: ignore[assignment] + for k, v in entry.items() + if k in _BLOCKING_REQUIRES_KEYS and isinstance(v, list) + } + + # Emit WARNING for requires-skills (read-only; never a skip). + req_skills = entry.get("requires-skills") + if isinstance(req_skills, list) and req_skills: + _warn_requires_skills(skill_name, req_skills) + + # Check blocking deps; skip on first missing. + missing = _skill_missing_dependency(requires_block) + if missing is not None: + kind, dep_name = missing + print(f"[skipped: {skill_name}: missing {kind} {dep_name}]") + continue + + _install_managed_tree(skill_template, skills_dir / skill_name, version) + count += 1 return count diff --git a/src/mapify_cli/schemas.py b/src/mapify_cli/schemas.py index cbf985d..df0c119 100644 --- a/src/mapify_cli/schemas.py +++ b/src/mapify_cli/schemas.py @@ -688,7 +688,9 @@ def load_and_validate( # Single authority for the four requires-* field names; consumers DERIVE from this # rather than hardcoding the list (see architecture-patterns: Single-Source Schema Dict). -SKILL_REQUIREMENTS_KEYS = tuple(SKILL_REQUIREMENTS_SCHEMA["properties"]) +_skill_req_props = SKILL_REQUIREMENTS_SCHEMA["properties"] +assert isinstance(_skill_req_props, dict) # runtime guard; schema is always a dict +SKILL_REQUIREMENTS_KEYS: tuple[str, ...] = tuple(_skill_req_props) ARTIFACT_STAGE_SCHEMA = { diff --git a/tests/test_file_copier.py b/tests/test_file_copier.py new file mode 100644 index 0000000..60e9d8f --- /dev/null +++ b/tests/test_file_copier.py @@ -0,0 +1,265 @@ +"""Tests for create_skill_files host-conditional pre-install skip (ST-004). + +Covers VC1-VC4: + VC1: missing blocking dep -> skip + print message; no files installed. + VC2: all deps present -> identical file set/count as baseline (identity). + VC3: upgrade-path guard fires when called directly with monkeypatched which. + VC4: unit tests for _skill_missing_dependency (pip/env/requires-skills/happy). +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent / "src")) + +from mapify_cli.delivery.file_copier import ( + _skill_missing_dependency, + create_skill_files, + get_templates_dir, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _installed_skill_dirs(base: Path) -> set[str]: + """Return the set of installed skill subdirectory names under .claude/skills/.""" + skills_dir = base / ".claude" / "skills" + if not skills_dir.exists(): + return set() + return {p.name for p in skills_dir.iterdir() if p.is_dir()} + + +def _expected_all_skill_dirs() -> set[str]: + """Return the set of skill dir names present in the shipped templates.""" + templates_dir = get_templates_dir() + skills_template_dir = templates_dir / "skills" + return { + p.name + for p in skills_template_dir.iterdir() + if p.is_dir() and p.name != "__pycache__" + } + + +# --------------------------------------------------------------------------- +# (a) VC1: missing blocking dep -> skip, no files installed, message printed +# --------------------------------------------------------------------------- + +class TestVC1MissingDepSkip: + def test_vc1_missing_cmd_skips_skill_and_prints_message( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """map-state requires-cmd:[git]; patching _REQUIRES_CHECKER["requires-cmd"] skips it.""" + import mapify_cli.delivery.file_copier as fc + + real_cmd_checker = fc._REQUIRES_CHECKER["requires-cmd"] + + def patched_cmd_checker(name: str) -> bool: + if name == "git": + return False + return real_cmd_checker(name) + + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-cmd", patched_cmd_checker) + + count = create_skill_files(tmp_path) + installed = _installed_skill_dirs(tmp_path) + out = capsys.readouterr().out + + # map-state must NOT be installed + assert "map-state" not in installed, ( + "map-state should be skipped when 'git' is not on PATH" + ) + # All other skills should still be installed (only map-state has requires-cmd:git) + all_skills = _expected_all_skill_dirs() + expected_installed = all_skills - {"map-state"} + assert installed == expected_installed, ( + f"Expected {expected_installed}, got {installed}" + ) + # Count must be total-1 + assert count == len(all_skills) - 1, ( + f"Expected count={len(all_skills) - 1}, got {count}" + ) + # Exact skip message must appear in stdout + assert "[skipped: map-state: missing cmd git]" in out, ( + f"Expected skip message in stdout; got: {out!r}" + ) + + +# --------------------------------------------------------------------------- +# (b) VC2: all deps present -> identical file set and count (happy-path identity) +# --------------------------------------------------------------------------- + +class TestVC2DepsPresent: + def test_vc2_all_deps_present_installs_all_skills( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """No monkeypatching: all shipped skills must be installed.""" + all_skills = _expected_all_skill_dirs() + count = create_skill_files(tmp_path) + installed = _installed_skill_dirs(tmp_path) + out = capsys.readouterr().out + + assert installed == all_skills, ( + f"Expected all skill dirs {all_skills}, got {installed}" + ) + assert count == len(all_skills), ( + f"Expected count={len(all_skills)}, got {count}" + ) + # No skip messages when all deps are present + assert "[skipped:" not in out, ( + f"Unexpected skip message in stdout: {out!r}" + ) + + +# --------------------------------------------------------------------------- +# (c) VC3: upgrade-path guard fires when create_skill_files called directly +# --------------------------------------------------------------------------- + +class TestVC3UpgradePathGuard: + def test_vc3_upgrade_path_guard_fires_on_missing_cmd( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """Calling create_skill_files directly (as upgrade does) must trigger guard.""" + import mapify_cli.delivery.file_copier as fc + + # First install without patching so there IS an existing installation. + create_skill_files(tmp_path) + capsys.readouterr() # discard first-install output + + # Simulate upgrade: call create_skill_files again with git missing. + monkeypatch.setitem( + fc._REQUIRES_CHECKER, "requires-cmd", lambda name: name != "git" + ) + + count2 = create_skill_files(tmp_path) + out2 = capsys.readouterr().out + + all_skills = _expected_all_skill_dirs() + assert count2 == len(all_skills) - 1, ( + "Upgrade path: count must exclude skipped map-state" + ) + assert "[skipped: map-state: missing cmd git]" in out2, ( + f"Upgrade path: skip message must appear; got: {out2!r}" + ) + + +# --------------------------------------------------------------------------- +# (d) VC4: unit tests for _skill_missing_dependency +# --------------------------------------------------------------------------- + +class TestVC4SkillMissingDependency: + def test_vc4_returns_none_when_no_requires(self) -> None: + assert _skill_missing_dependency({}) is None + + def test_vc4_returns_none_when_all_present( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-cmd", lambda _: True) + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-pip", lambda _: True) + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-env", lambda _: True) + result = _skill_missing_dependency({ + "requires-cmd": ["git"], + "requires-pip": ["yaml"], + "requires-env": ["HOME"], + }) + assert result is None + + def test_vc4_missing_pip_returns_pip_kind( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-pip", lambda _: False) + result = _skill_missing_dependency({"requires-pip": ["some_nonexistent_pkg"]}) + assert result is not None + kind, name = result + assert kind == "pip" + assert name == "some_nonexistent_pkg" + + def test_vc4_missing_env_returns_env_kind( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem( + fc._REQUIRES_CHECKER, + "requires-env", + lambda name: name != "MISSING_VAR_XYZ_12345", + ) + result = _skill_missing_dependency({"requires-env": ["MISSING_VAR_XYZ_12345"]}) + assert result is not None + kind, name = result + assert kind == "env" + assert name == "MISSING_VAR_XYZ_12345" + + def test_vc4_requires_skills_is_warn_only_returns_none(self) -> None: + """requires-skills must never cause a skip (returns None).""" + # _skill_missing_dependency only checks blocking keys; requires-skills + # is explicitly excluded from _BLOCKING_REQUIRES_KEYS. + result = _skill_missing_dependency({"requires-skills": ["map-state"]}) + assert result is None, ( + "requires-skills is warn-only and must never cause " + "_skill_missing_dependency to return a (kind, name) tuple" + ) + + def test_vc4_missing_cmd_returns_cmd_kind( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-cmd", lambda _: False) + result = _skill_missing_dependency({"requires-cmd": ["nonexistent-tool-xyz"]}) + assert result is not None + kind, name = result + assert kind == "cmd" + assert name == "nonexistent-tool-xyz" + + def test_vc4_first_missing_dep_wins_order_cmd_before_pip( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """requires-cmd is checked before requires-pip.""" + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-cmd", lambda _: False) + monkeypatch.setitem(fc._REQUIRES_CHECKER, "requires-pip", lambda _: False) + result = _skill_missing_dependency({ + "requires-cmd": ["git"], + "requires-pip": ["yaml"], + }) + assert result is not None + kind, _ = result + assert kind == "cmd", "cmd must be checked before pip" + + def test_vc4_env_check_reads_name_only_not_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Security: _check_requires_env must use 'name in os.environ', not read value.""" + import mapify_cli.delivery.file_copier as fc + + # Set a sentinel variable whose value we must never observe. + sentinel_name = "MAP_SECURITY_TEST_VAR_DO_NOT_READ" + monkeypatch.setenv(sentinel_name, "SECRET_VALUE") + + # The check must return True (name is present) without accessing the value. + result = fc._check_requires_env(sentinel_name) + assert result is True + + # Also verify absent var returns False. + monkeypatch.delenv(sentinel_name, raising=False) + result2 = fc._check_requires_env(sentinel_name) + assert result2 is False From 9fea46885f95684a439760c9578296f69bf30cb6 Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 14:43:59 +0300 Subject: [PATCH 5/6] fix(skills): address code-review findings on the requires-* gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness/integrity: - Scanner (test_skills_consistency): detect here-doc openers AFTER comment + quote stripping with a quote-parity guard, so a `< --- src/mapify_cli/delivery/codex_copier.py | 41 +++- src/mapify_cli/delivery/file_copier.py | 112 ++++++++-- tests/test_file_copier.py | 114 +++++++++- tests/test_skills_consistency.py | 271 ++++++++---------------- 4 files changed, 334 insertions(+), 204 deletions(-) diff --git a/src/mapify_cli/delivery/codex_copier.py b/src/mapify_cli/delivery/codex_copier.py index 2564194..197c43c 100644 --- a/src/mapify_cli/delivery/codex_copier.py +++ b/src/mapify_cli/delivery/codex_copier.py @@ -11,7 +11,15 @@ import shutil from pathlib import Path -from mapify_cli.delivery.file_copier import _get_version, get_templates_dir +from mapify_cli.delivery.file_copier import ( + _extract_requires_block, + _get_version, + _load_template_skill_catalog, + _prune_catalog_entries, + _skill_missing_dependency, + _warn_requires_skills, + get_templates_dir, +) from mapify_cli.delivery.managed_file_copier import copy_managed_file @@ -126,12 +134,37 @@ def create_codex_files(project_path: Path) -> dict[str, int]: # ------------------------------------------------------------------ skills_src = codex_templates / "skills" if skills_src.exists(): - for skill_dir in skills_src.iterdir(): - if not skill_dir.is_dir(): + # Same host-conditional requires-* gate as the Claude provider, so the + # requires-* contract is enforced identically across providers. The + # Codex skills tree ships no skill-rules.json today, so the catalog is + # empty and nothing is gated — but a future Codex skill declaring + # requires-* is honoured without re-implementing the gate here. + skill_catalog = _load_template_skill_catalog(skills_src) + skipped: list[str] = [] + for skill_dir in sorted(skills_src.iterdir()): + if not skill_dir.is_dir() or skill_dir.name == "__pycache__": continue - skill_dst = agents_dir / "skills" / skill_dir.name + + skill_name = skill_dir.name + entry = skill_catalog.get(skill_name, {}) + requires_block = _extract_requires_block(skill_name, entry) + + req_skills = entry.get("requires-skills") if isinstance(entry, dict) else None + if isinstance(req_skills, list) and req_skills: + _warn_requires_skills(skill_name, req_skills) + + missing = _skill_missing_dependency(requires_block) + if missing is not None: + kind, dep_name = missing + print(f"[skipped: {skill_name}: missing {kind} {dep_name}]") + skipped.append(skill_name) + continue + + skill_dst = agents_dir / "skills" / skill_name counts["skills"] += _copy_tree(skill_dir, skill_dst, version) + _prune_catalog_entries(agents_dir / "skills" / "skill-rules.json", skipped) + # ------------------------------------------------------------------ # 2. Agents (*.toml) — watched (fence-aware) # ------------------------------------------------------------------ diff --git a/src/mapify_cli/delivery/file_copier.py b/src/mapify_cli/delivery/file_copier.py index 983314b..8d74e35 100644 --- a/src/mapify_cli/delivery/file_copier.py +++ b/src/mapify_cli/delivery/file_copier.py @@ -22,7 +22,11 @@ create_reflector_content, create_documentation_reviewer_content, ) -from mapify_cli.schemas import SKILL_REQUIREMENTS_KEYS +from mapify_cli.schemas import ( + SKILL_REQUIREMENTS_KEYS, + SKILL_REQUIREMENTS_SCHEMA, + validate_artifact, +) _IGNORED_TEMPLATE_NAMES = {"__pycache__", ".DS_Store"} @@ -30,11 +34,12 @@ # Ordered check dispatch for blocking requires-* keys. # _BLOCKING_REQUIRES_KEYS is derived from SKILL_REQUIREMENTS_KEYS (schema authority). -# The module-level assertion below enforces that _REQUIRES_CHECKER covers EVERY +# The module-level check below enforces that _REQUIRES_CHECKER covers EVERY # blocking key derived from the schema: adding a new blocking key to -# SKILL_REQUIREMENTS_SCHEMA raises AssertionError at import time unless a +# SKILL_REQUIREMENTS_SCHEMA raises RuntimeError at import time unless a # corresponding checker is added here — the invariant is mechanically enforced, -# not just documented. +# not just documented. (A bare ``assert`` would be stripped under ``python -O``, +# silently turning the guarantee into a no-op, so we raise explicitly.) # requires-skills is warn-only (not a skip), handled separately. _BLOCKING_REQUIRES_KEYS = { k for k in SKILL_REQUIREMENTS_KEYS if k != "requires-skills" @@ -42,7 +47,17 @@ def _check_requires_cmd(name: str) -> bool: - """Return True if CLI command *name* is available on PATH.""" + """Return True if CLI command *name* is available on PATH. + + Mirrors ``check_tool()`` in ``mapify_cli.__init__``: the Claude CLI may be + installed only at ``~/.claude/local/claude`` (not on PATH), so that location + counts as present. Kept deliberately in sync — importing ``check_tool`` here + would create a circular import (``mapify_cli.__init__`` imports this module). + """ + if name == "claude": + claude_local = Path.home() / ".claude" / "local" / "claude" + if claude_local.is_file(): + return True return shutil.which(name) is not None @@ -71,11 +86,13 @@ def _check_requires_env(name: str) -> bool: # Enforced invariant: every blocking key derived from the schema must have a # checker entry here. Adding a new blocking key to SKILL_REQUIREMENTS_SCHEMA -# without a matching checker raises AssertionError at import time. -assert _BLOCKING_REQUIRES_KEYS == set(_REQUIRES_CHECKER), ( - f"_REQUIRES_CHECKER is out of sync with SKILL_REQUIREMENTS_KEYS; " - f"missing checkers for: {_BLOCKING_REQUIRES_KEYS - set(_REQUIRES_CHECKER)}" -) +# without a matching checker raises RuntimeError at import time. Uses an +# explicit raise (not ``assert``) so the guarantee survives ``python -O``. +if _BLOCKING_REQUIRES_KEYS != set(_REQUIRES_CHECKER): + raise RuntimeError( + "_REQUIRES_CHECKER is out of sync with SKILL_REQUIREMENTS_KEYS; " + f"missing checkers for: {_BLOCKING_REQUIRES_KEYS - set(_REQUIRES_CHECKER)}" + ) def _skill_missing_dependency(requires_block: dict[str, list[str]]) -> tuple[str, str] | None: @@ -101,6 +118,67 @@ def _warn_requires_skills(skill_name: str, skill_names: list[str]) -> None: print(f"[warning: {skill_name}: requires skill {dep}]") +def _extract_requires_block(skill_name: str, entry: object) -> dict[str, list[str]]: + """Return the blocking requires-* sub-block (list-valued) for a skill entry. + + Defensive against a malformed catalog so a single bad entry never corrupts + the install: + - a non-dict entry yields ``{}`` (no requirements; never raises); + - the requires-* sub-block is validated against SKILL_REQUIREMENTS_SCHEMA and + any violation is surfaced as a ``[warning: ...]`` (never silently dropped), + so a typo'd scalar like ``"requires-cmd": "git"`` cannot quietly disable the + gate. This is where the schema earns its keep at install time. + + Only well-formed list-of-strings blocking keys are returned for enforcement. + """ + if not isinstance(entry, dict): + return {} + sub_block = {k: entry[k] for k in SKILL_REQUIREMENTS_KEYS if k in entry} + if sub_block: + valid, errors = validate_artifact(sub_block, SKILL_REQUIREMENTS_SCHEMA) + if not valid: + print( + f"[warning: {skill_name}: malformed requires-* in skill-rules.json: " + f"{'; '.join(errors)}]" + ) + return { + k: v + for k, v in sub_block.items() + if k in _BLOCKING_REQUIRES_KEYS + and isinstance(v, list) + and all(isinstance(item, str) for item in v) + } + + +def _prune_catalog_entries(catalog_path: Path, skill_names: list[str]) -> None: + """Remove *skill_names* from the INSTALLED skill-rules.json. + + Keeps the installed catalog consistent with the on-disk skill set: a skill + skipped for a missing host dependency must not remain advertised in + skill-rules.json (a listed-but-absent skill would dangle when its triggers + fire). Preserves the ``_map_managed`` sentinel and JSON formatting written by + the managed-file copier. No-op on read/parse error or if nothing matched. + """ + if not skill_names or not catalog_path.exists(): + return + try: + data = json.loads(catalog_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return + skills = data.get("skills") if isinstance(data, dict) else None + if not isinstance(skills, dict): + return + changed = False + for name in skill_names: + if name in skills: + del skills[name] + changed = True + if changed: + catalog_path.write_text( + json.dumps(data, indent=2, ensure_ascii=False) + "\n", encoding="utf-8" + ) + + def _get_version() -> str: """Get current mapify-cli version for metadata injection.""" try: @@ -284,20 +362,17 @@ def create_skill_files(project_path: Path) -> int: _install_managed_file(top_src, skills_dir / top_name, version) # Copy each skill directory, fence-aware per file (watched category). + skipped: list[str] = [] for skill_template in sorted(skills_template_dir.iterdir()): if not (skill_template.is_dir() and skill_template.name != "__pycache__"): continue skill_name = skill_template.name entry = skill_catalog.get(skill_name, {}) - requires_block: dict[str, list[str]] = { - k: v # type: ignore[assignment] - for k, v in entry.items() - if k in _BLOCKING_REQUIRES_KEYS and isinstance(v, list) - } + requires_block = _extract_requires_block(skill_name, entry) # Emit WARNING for requires-skills (read-only; never a skip). - req_skills = entry.get("requires-skills") + req_skills = entry.get("requires-skills") if isinstance(entry, dict) else None if isinstance(req_skills, list) and req_skills: _warn_requires_skills(skill_name, req_skills) @@ -306,11 +381,16 @@ def create_skill_files(project_path: Path) -> int: if missing is not None: kind, dep_name = missing print(f"[skipped: {skill_name}: missing {kind} {dep_name}]") + skipped.append(skill_name) continue _install_managed_tree(skill_template, skills_dir / skill_name, version) count += 1 + # Keep the installed catalog consistent with the on-disk skill set: + # a skill skipped above must not stay advertised in skill-rules.json. + _prune_catalog_entries(skills_dir / "skill-rules.json", skipped) + return count diff --git a/tests/test_file_copier.py b/tests/test_file_copier.py index 60e9d8f..a783583 100644 --- a/tests/test_file_copier.py +++ b/tests/test_file_copier.py @@ -9,6 +9,7 @@ from __future__ import annotations +import json import sys from pathlib import Path @@ -101,9 +102,20 @@ class TestVC2DepsPresent: def test_vc2_all_deps_present_installs_all_skills( self, tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], ) -> None: - """No monkeypatching: all shipped skills must be installed.""" + """All deps present -> every shipped skill installed (identity). + + Force every dependency checker to report present so the assertion is + host-independent: a CI image without git on PATH must not make this + happy-path identity test fail (map-state declares requires-cmd:[git]). + """ + import mapify_cli.delivery.file_copier as fc + + for kind in fc._REQUIRES_CHECKER: + monkeypatch.setitem(fc._REQUIRES_CHECKER, kind, lambda _: True) + all_skills = _expected_all_skill_dirs() count = create_skill_files(tmp_path) installed = _installed_skill_dirs(tmp_path) @@ -263,3 +275,103 @@ def test_vc4_env_check_reads_name_only_not_value( monkeypatch.delenv(sentinel_name, raising=False) result2 = fc._check_requires_env(sentinel_name) assert result2 is False + + +# --------------------------------------------------------------------------- +# Catalog/dir consistency: a skipped skill is pruned from the installed +# skill-rules.json so the catalog never advertises an absent skill. +# --------------------------------------------------------------------------- + + +def _installed_catalog_skills(base: Path) -> set[str]: + """Skill names listed in the installed .claude/skills/skill-rules.json.""" + catalog = base / ".claude" / "skills" / "skill-rules.json" + data = json.loads(catalog.read_text(encoding="utf-8")) + return set(data.get("skills", {}).keys()) + + +class TestCatalogConsistencyOnSkip: + def test_skipped_skill_pruned_from_installed_catalog( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """map-state skipped (git missing) must NOT remain in installed catalog.""" + import mapify_cli.delivery.file_copier as fc + + monkeypatch.setitem( + fc._REQUIRES_CHECKER, "requires-cmd", lambda name: name != "git" + ) + + create_skill_files(tmp_path) + + installed_dirs = _installed_skill_dirs(tmp_path) + catalog_skills = _installed_catalog_skills(tmp_path) + + assert "map-state" not in installed_dirs, "map-state dir should be skipped" + assert "map-state" not in catalog_skills, ( + "skipped map-state must be pruned from installed skill-rules.json " + "so the catalog matches the on-disk skill set" + ) + # The catalog and the on-disk skill dirs must agree. + assert catalog_skills == installed_dirs, ( + f"catalog {catalog_skills} != installed dirs {installed_dirs}" + ) + + def test_all_present_keeps_full_catalog( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """No skips -> installed catalog is unchanged (no pruning, no drift).""" + import mapify_cli.delivery.file_copier as fc + + for kind in fc._REQUIRES_CHECKER: + monkeypatch.setitem(fc._REQUIRES_CHECKER, kind, lambda _: True) + + create_skill_files(tmp_path) + catalog_skills = _installed_catalog_skills(tmp_path) + assert "map-state" in catalog_skills + assert catalog_skills == _installed_skill_dirs(tmp_path) + + +# --------------------------------------------------------------------------- +# Robustness: a malformed catalog entry must not crash the install and must +# not silently disable the gate. +# --------------------------------------------------------------------------- + +class TestMalformedCatalogRobustness: + def test_non_dict_entry_yields_no_requirements(self) -> None: + """A non-dict skill entry is treated as having no requirements (no crash).""" + import mapify_cli.delivery.file_copier as fc + + assert fc._extract_requires_block("x", None) == {} + assert fc._extract_requires_block("x", ["requires-cmd"]) == {} + assert fc._extract_requires_block("x", "git") == {} + + def test_scalar_requires_value_is_warned_not_silently_dropped( + self, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A typo'd scalar requires-cmd must surface a warning, not vanish silently.""" + import mapify_cli.delivery.file_copier as fc + + # "git" (a string, not ["git"]) violates SKILL_REQUIREMENTS_SCHEMA. + block = fc._extract_requires_block("bad-skill", {"requires-cmd": "git"}) + out = capsys.readouterr().out + + # Malformed value is not enforced (not a list) ... + assert block == {} + # ... but it is surfaced rather than silently swallowed. + assert "bad-skill" in out and "malformed requires-*" in out, ( + f"expected a malformed-requires warning; got: {out!r}" + ) + + def test_wellformed_block_passes_through(self) -> None: + import mapify_cli.delivery.file_copier as fc + + block = fc._extract_requires_block( + "ok-skill", {"requires-cmd": ["git"], "requires-skills": ["map-state"]} + ) + # Only blocking, list-valued keys are returned; requires-skills excluded. + assert block == {"requires-cmd": ["git"]} diff --git a/tests/test_skills_consistency.py b/tests/test_skills_consistency.py index f322e6e..5bf2182 100644 --- a/tests/test_skills_consistency.py +++ b/tests/test_skills_consistency.py @@ -14,6 +14,7 @@ import copy import json import re +import sys from pathlib import Path from typing import Any @@ -115,181 +116,19 @@ } ) -# Standard-library top-level module names (Python 3.11). -# Only third-party imports are flagged for requires-pip. -_STDLIB_MODULES: frozenset[str] = frozenset( - { - "__future__", - "_thread", - "abc", - "argparse", - "ast", - "asyncio", - "base64", - "binascii", - "builtins", - "calendar", - "cmath", - "cmd", - "code", - "codecs", - "collections", - "colorsys", - "compileall", - "concurrent", - "configparser", - "contextlib", - "copy", - "copyreg", - "csv", - "ctypes", - "curses", - "dataclasses", - "datetime", - "dbm", - "decimal", - "difflib", - "dis", - "doctest", - "email", - "encodings", - "enum", - "errno", - "faulthandler", - "fcntl", - "filecmp", - "fileinput", - "fnmatch", - "fractions", - "ftplib", - "functools", - "gc", - "getopt", - "getpass", - "gettext", - "glob", - "gzip", - "hashlib", - "heapq", - "hmac", - "html", - "http", - "idlelib", - "imaplib", - "importlib", - "inspect", - "io", - "ipaddress", - "itertools", - "json", - "keyword", - "linecache", - "locale", - "logging", - "lzma", - "mailbox", - "marshal", - "math", - "mimetypes", - "mmap", - "modulefinder", - "multiprocessing", - "netrc", - "numbers", - "operator", - "optparse", - "os", - "pathlib", - "pdb", - "pkgutil", - "platform", - "plistlib", - "poplib", - "posix", - "posixpath", - "pprint", - "profile", - "pstats", - "pty", - "py_compile", - "pyclbr", - "pydoc", - "queue", - "quopri", - "random", - "re", - "readline", - "reprlib", - "resource", - "rlcompleter", - "runpy", - "sched", - "secrets", - "select", - "selectors", - "shelve", - "shlex", - "shutil", - "signal", - "site", - "smtplib", - "socket", - "socketserver", - "sqlite3", - "ssl", - "stat", - "statistics", - "string", - "stringprep", - "struct", - "subprocess", - "sys", - "sysconfig", - "syslog", - "tabnanny", - "tarfile", - "telnetlib", - "tempfile", - "termios", - "test", - "textwrap", - "threading", - "time", - "timeit", - "tkinter", - "token", - "tokenize", - "tomllib", - "trace", - "traceback", - "tracemalloc", - "tty", - "turtle", - "types", - "typing", - "unicodedata", - "unittest", - "urllib", - "uuid", - "venv", - "warnings", - "wave", - "weakref", - "webbrowser", - "wsgiref", - "xml", - "xmlrpc", - "zipapp", - "zipfile", - "zipimport", - "zlib", - "zoneinfo", - } -) +# Standard-library top-level module names, sourced from the running +# interpreter (sys.stdlib_module_names, available since Python 3.10; the +# project targets 3.11+). Only third-party imports are flagged for +# requires-pip. Deriving from the interpreter avoids the drift a +# hand-maintained list incurs on every Python upgrade. +_STDLIB_MODULES: frozenset[str] = sys.stdlib_module_names # ALL_CAPS tokens are shell variable references (e.g. BRANCH, PLAN_FILE), not commands. _ALL_CAPS_RE = re.compile(r"^[A-Z_][A-Z0-9_]*$") +# Here-document opener: < set[str]: in_sq_block = False continue - # Detect heredoc opener: <<[-]WORD, <<[-]'WORD', <<[-]"WORD" - # Must be checked before scanning so we don't scan the opener line's - # here-doc body (content after the << is on the NEXT line). - hd_match = re.search(r"<<(-?)\s*['\"]?([A-Za-z_][A-Za-z0-9_]*)['\"]?", line) - if hd_match: - in_heredoc = True - heredoc_indented = hd_match.group(1) == "-" - heredoc_term = hd_match.group(2) - # Fall through: still scan the opener line itself (the command before <<). - # Skip pure comment lines. if stripped.startswith("#"): continue # Strip inline tail comment (conservative: only ' #' with leading whitespace). - clean = re.sub(r"\s#.*$", "", line) + line_no_comment = re.sub(r"\s#.*$", "", line) + + # Detect a here-document opener (< None: assert "git" in cmds, "git outside the heredoc must still be detected" +def test_scanner_heredoc_token_in_comment_or_string_does_not_swallow( + tmp_path: Path, +) -> None: + """A `< None: + """Real heredocs with <<'EOF', <<"EOF", and <<- still suppress their bodies.""" + script = tmp_path / "real_heredocs.sh" + script.write_text( + "#!/usr/bin/env bash\n" + 'cat <<"END1"\n' + "docker run a\n" + "END1\n" + "cat <<-END2\n" + "\tpodman run b\n" + "\tEND2\n" + "git status\n", + encoding="utf-8", + ) + cmds = _scan_sh_commands(script) + assert "docker" not in cmds and "podman" not in cmds, ( + "bodies of <<\"END1\" and <<-END2 heredocs must be suppressed" + ) + assert "git" in cmds + + # --------------------------------------------------------------------------- # Teeth self-test — under-declaration actually fails (not vacuously always-green) # --------------------------------------------------------------------------- From d4bbf2524e0c5db222f8f7f4983b67dc8ecbbf54 Mon Sep 17 00:00:00 2001 From: Mikhail Petrov Date: Tue, 2 Jun 2026 14:50:48 +0300 Subject: [PATCH 6/6] fix(build): drop redundant templates_src force-include (duplicate wheel path) templates_src/**/*.jinja already lands in the wheel under mapify_cli/templates_src via packages=["src/mapify_cli"] plus the global [tool.hatch.build] include/artifacts patterns. The force-include added the same files a second time at the identical archive path; newer hatchling rejects this with "A second file is being added to the wheel archive at the same path: mapify_cli/templates_src/CLAUDE.md.jinja" (it previously deduped silently, which is why main was green). Removing the force-include fixes the build; verified the wheel still contains all 95 templates_src .jinja files and the generated templates/ tree, with no duplicates. Co-Authored-By: Claude Opus 4.8 (1M context) --- pyproject.toml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index f9212e9..58d57d9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -70,9 +70,11 @@ artifacts = [ [tool.hatch.build.targets.wheel] packages = ["src/mapify_cli"] - -[tool.hatch.build.targets.wheel.force-include] -"src/mapify_cli/templates_src" = "mapify_cli/templates_src" +# NOTE: do NOT force-include src/mapify_cli/templates_src here. It already lands +# in the wheel under mapify_cli/templates_src via `packages` + the global +# [tool.hatch.build] include/artifacts patterns for templates_src/**/*.jinja. +# A force-include duplicates every .jinja at the same archive path, which newer +# hatchling rejects with "A second file is being added ... at the same path". [tool.hatch.build.targets.sdist] include = [