From b6e68e9a2b473d18205644e4f172abaf304eeadb Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Sun, 16 Aug 2026 16:39:29 +0000 Subject: [PATCH] fix(kyberforge): close the vacuous-pass paths in the ADR-0020 gate scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the gates could report green having measured nothing. All three were invisible to a passing test suite, because pre-commit prints nothing at all for a hook that exits 0 — a gate that declines to check and a gate that checked and passed produce the identical signal. - A UTF-8 BOM, a leading blank line, a trailing space after a `---` marker or CRLF line endings defeated the `^---\n` frontmatter matcher. Every ADR-0020 check was then skipped and the file passed: measured at the time, a 550-character description with a 1,000-word body exited 0 behind a BOM. All four shapes are now tolerated, and frontmatter that genuinely cannot be parsed is a hard ERROR rather than a silent skip. - An agent file with a valueless `description:` followed by another key let a line regex capture the *next* key, which looked non-empty, so the missing-or-empty branch never fired and every gate below it early-returned on the empty folded value — zero output, exit 0, on a blocking gate. The one field this contract is entirely about was the one field a gate could fail to notice was absent. Presence is now decided on the YAML-folded value and nowhere else, and a missing or empty description is a hard FAIL in all three validators. - The hand-rolled frontmatter fallback disagreed with PyYAML across the FAIL boundary on folded scalars, so which reader happened to be available decided the verdict. A fallback that mis-parses a scalar shape reports a vacuous pass, which is worse than not running, so it is deleted: python3 and PyYAML are hard requirements that fail loudly with an install pointer. Boundary-target resolution no longer derives its universe from its own location. A `${BASH_SOURCE}`-relative repo root leaked this repo's 39-skill universe into every consumer repo running the hook through pre-commit, so a consumer skill routing to `skill-audit` resolved against a plugin it had never installed. The interim form resolved through `.claude/` and `.agents/`, which are gitignored `apm install` output — the same commit reported 2 dangling targets on a machine that had run the install and 6 on a fresh clone. Resolution now walks up from the file being checked to an authoring root (nearest ancestor holding `plugins/*/.apm/{skills,agents}`, else the nearest `.git`, in two passes so a nested `.git` cannot outrank a real monorepo root); the universe is every skill and agent under `/plugins/*/` plus the file's own apm package and that package's declared `dependencies.apm`. Deployed trees are consulted only when no authoring root exists at all — the consumer case. One commit now gets one verdict, which a gate shipping hot with no baseline file has to. Narrowed in the same pass: a routing target inferred from the prose boundary form and corroborated by nothing else reports at SUGGESTION instead of blocking. A blocking check with no escape hatch is the wrong trade when the inference from prose is the weak part of it. New deterministic checks, all previously untested or absent: every `references/.md` a body names must exist (ERROR — a broken pointer is not a style opinion); a description with no boundary clause at all, a Gotchas section over five entries, and a Gotchas section over 25% of the body are SUGGESTIONs. Where no universe can be determined the target check prints `INFO ... DID NOT RUN` rather than passing quietly. Each prose-scanning check needed its own false-positive fix — a fenced example of a Gotchas section was being read as the section itself — and those fixes are pinned rather than assumed. The resolver is one block copied verbatim into all three scripts between BEGIN/END markers, because a cache-installed plugin's scripts cannot read outside their own plugin directory. Nothing asserted the copies were still identical; a one-line edit to a single copy passed every constant-agreement assertion, since constants are not what drifts. Tests land here rather than in a later commit. The existing suites assert the old behaviour and go red against these scripts, so splitting them would leave a commit whose own `run-tests` pre-push gate fails in isolation. Refs: ADR-0020 --- .../skills/agent-audit/scripts/validate.sh | 926 +++++++++++++-- .../skills/agent-audit/tests/validate.bats | 42 +- .../skills/skill-audit/scripts/validate.sh | 1007 ++++++++++++++--- .../skills/skill-audit/tests/validate.bats | 88 +- .../skills/agent-audit/scripts/validate.sh | 926 +++++++++++++-- .../skills/skill-audit/scripts/validate.sh | 1007 ++++++++++++++--- scripts/skill-size-check.sh | 943 ++++++++++++--- tests/test-adr0020-body-checks.sh | 406 +++++++ tests/test-adr0020-contract.sh | 331 ++++++ tests/test-adr0020-differential.sh | 365 ++++++ tests/test-adr0020-frontmatter.sh | 286 +++++ tests/test-adr0020-targets.sh | 409 +++++++ tests/test-skill-size-check.sh | 163 ++- 13 files changed, 6158 insertions(+), 741 deletions(-) create mode 100755 tests/test-adr0020-body-checks.sh create mode 100755 tests/test-adr0020-contract.sh create mode 100755 tests/test-adr0020-differential.sh create mode 100755 tests/test-adr0020-frontmatter.sh create mode 100755 tests/test-adr0020-targets.sh diff --git a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh index 454e253..5738db4 100755 --- a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh @@ -36,12 +36,38 @@ if [[ $# -lt 1 ]]; then exit 1 fi +# PyYAML is a HARD dependency, not a nice-to-have. The description VALUE has to +# be measured after YAML folding is resolved, and the hand-rolled reader that +# used to stand in for PyYAML disagreed with it across the 400-character FAIL +# boundary — same description, two verdicts, depending on which reader ran. +# Refusing to start is the only honest option; the repo's jq / apm / vale +# dependencies are declared the same way. +# Check the interpreter separately from the library: `python3 -c` fails the same +# way whether python3 is missing or PyYAML is, and reporting the wrong missing +# dependency sends the reader to install the wrong thing. +if ! command -v python3 > /dev/null 2>&1; then + echo "Error: python3 is required but was not found on PATH." >&2 + echo " Why: skipping the ADR-0020 description and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: install python3 (pre-commit itself is a Python application, so it is almost certainly already present)." >&2 + exit 1 +fi + +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "Error: PyYAML is required but is not importable by python3." >&2 + echo " Why: skipping the ADR-0020 description and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" python3 -u - "$1" "$SCRIPT_DIR" <<'PYTHON' import sys import os import re +import glob + +import yaml agent_file = os.path.abspath(sys.argv[1]) script_dir = sys.argv[2] @@ -65,8 +91,18 @@ if not os.path.isfile(inv_path): print(f"Error: field-inventory.md not found at {inv_path}", file=sys.stderr) sys.exit(2) -with open(inv_path) as f: - inv_content = f.read() +# Encoding is pinned to UTF-8 rather than inherited from the locale: under +# LC_ALL=C the inherited default is ASCII, and this file legitimately carries +# non-ASCII prose. read_text() in the shared resolver block below does the same +# thing for every other file; this one is read before that block is defined. +try: + with open(inv_path, encoding='utf-8') as f: + inv_content = f.read() +except UnicodeDecodeError as exc: + print(f"Error: field-inventory.md at {inv_path} is not valid UTF-8 " + f"({exc.reason} at byte {exc.start}) — re-save it as UTF-8.", + file=sys.stderr) + sys.exit(2) def parse_section_tokens(content, section_name): lines = content.splitlines() @@ -112,23 +148,716 @@ failed = False suggestions = [] def fail(msg): + # stderr, matching scripts/skill-size-check.sh's ERROR routing. All three + # scripts in the ADR-0020 family now agree: findings that fail the run go to + # stderr, everything advisory (SUGGESTION / INFO) goes to stdout. Both repo + # callers (check-apm-agents-valid.sh, check-scope-walkup-sync.sh) capture + # `2>&1`, so nothing a human reads moves. global failed failed = True - print(f"FAIL {msg}") + print(f"FAIL {msg}", file=sys.stderr) def suggest(msg): suggestions.append(msg) +def info(msg): + # A check that DECLINED to run says so out loud, rather than passing + # silently. Silence is what let a whole gate family go missing unnoticed. + print(f"INFO {msg}") + PLACEHOLDER_RE = re.compile(r'(?/plugins/*/ — sibling plugins resolve, +# which is what a monorepo means, +# 2. the target's own apm package, +# 3. the packages that package DECLARES in apm.yml dependencies.apm. +# Deployed .claude/ and .agents/ trees are deliberately NOT consulted in that +# case. They are `apm install` output, gitignored, and present only on a machine +# that has run it: four cross-plugin targets in this repo (gitea-branches -> +# git-branches, gitea-branches -> git-history, gitea-issues -> git-branches, +# gitea-workflow -> git-workflow) resolved through .claude/skills/ alone, so the +# same commit measured 2 dangling targets on a developer machine and 6 on a +# fresh clone. A gate shipping hot with no baseline cannot give two answers. +# +# Deployed trees are used only when NO authoring root exists — the consumer +# case, where the file being checked lives in or beside a deployed tree and +# there is no monorepo to read. + + +def _is_fs_root(path): + return os.path.dirname(path) == path + + +def _collect_package(pkg_dir, names): + """Add every skill/agent name a package directory exposes, any layout.""" + for sub in ('.apm/skills/*/', 'skills/*/'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + names.add(os.path.basename(path.rstrip('/')).lower()) + for sub in ('.apm/agents/*.md', 'agents/*.md'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + base = os.path.basename(path) + if base.endswith('.agent.md'): + base = base[:-len('.agent.md')] + else: + base = base[:-len('.md')] + names.add(base.lower()) + + +def _apm_package_root(start_dir): + """Nearest ancestor that is an apm package root (apm.yml or .apm/). + + The filesystem root is never a candidate: a stray /.apm/skills/ — a + scaffolding test's leftover, say, and one really does exist on at least one + machine here — would otherwise become the package root of every path on it. + Capped at ten levels so a pathological path can't become a filesystem + crawl; that covers every real layout by a wide margin. + """ + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + return None + if (os.path.isfile(os.path.join(current, 'apm.yml')) + or os.path.isdir(os.path.join(current, '.apm'))): + return current + current = os.path.dirname(current) + return None + + +def _authoring_root(start_dir): + """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + + Two passes, not one interleaved walk: a nested .git (a submodule, a + worktree of a sub-package) must not win over a real plugins/ root further + up. Both passes stop before the filesystem root for the same reason + _apm_package_root does. + """ + for probe in ( + lambda d: bool(glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))): + current = os.path.abspath(start_dir) + for _ in range(12): + if _is_fs_root(current): + break + if probe(current): + return current + current = os.path.dirname(current) + return None + + +def _collect_authoring_root(root, names): + """Every plugin in the monorepo contributes its names.""" + for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + if os.path.isdir(pkg): + _collect_package(pkg, names) + + +def _declared_dependency_dirs(pkg_dir): + """Directories of the apm packages pkg_dir's manifest DECLARES. + + Reads dependencies.apm and resolves each entry to a directory on disk: + a monorepo-relative `path:` (against the package root and the nearest + ancestor manifest, which is the monorepo root) or an installed + apm_modules//. Entries that resolve to nothing are skipped — an + undeployed dependency contributes no names rather than an error. + """ + manifest = os.path.join(pkg_dir, 'apm.yml') + if not os.path.isfile(manifest): + return [] + try: + data = yaml.safe_load(read_text(manifest)) or {} + except Exception: + return [] + if not isinstance(data, dict): + return [] + deps = data.get('dependencies') + deps = deps.get('apm') if isinstance(deps, dict) else None + if not isinstance(deps, list): + return [] + + roots = [pkg_dir] + ancestor = os.path.dirname(os.path.abspath(pkg_dir)) + for _ in range(10): + if _is_fs_root(ancestor): + break + if os.path.isfile(os.path.join(ancestor, 'apm.yml')): + roots.append(ancestor) + break + ancestor = os.path.dirname(ancestor) + + found = [] + for entry in deps: + candidates = [] + if isinstance(entry, dict): + rel = entry.get('path') + name = entry.get('name') + if not name and rel: + name = os.path.basename(str(rel).rstrip('/')) + if rel: + candidates.extend(os.path.join(r, str(rel)) for r in roots) + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', str(name))) + elif isinstance(entry, str): + name = re.split(r'[#@]', entry)[0].strip().rstrip('/').split('/')[-1] + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', name)) + candidates.extend(os.path.join(r, 'plugins', name) for r in roots) + for candidate in candidates: + if os.path.isdir(candidate): + found.append(candidate) + return found + + +def _deployed_roots(start_dir): + """.claude/ and .agents/ trees above start_dir — what a host really sees. + + Consulted ONLY when no authoring root exists; see the section header. The + filesystem root is skipped for the same reason _apm_package_root skips it: + a stray /.claude/skills/ must not join every path's universe. + """ + found = [] + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + break + for name in ('.claude', '.agents'): + base = os.path.join(current, name) + if os.path.isdir(base): + found.append(base) + current = os.path.dirname(current) + return found + + +def known_targets(start_dir): + """Every skill/agent name a boundary clause in start_dir may name.""" + names = set() + start = os.path.abspath(start_dir) + + # Siblings: a cache-installed plugin and a deployed .claude/skills/ tree + # both put peers one level up, with no plugins/ directory above them. The + # grandparent is guarded against the filesystem root exactly like the two + # walk-up loops above — for a start dir of /skills/ the grandparent is + # `/`, and collecting there picks up this machine's stray /.apm/skills/. + parent = os.path.dirname(start) + grandparent = os.path.dirname(parent) + if (os.path.basename(parent) in ('skills', 'agents') + and os.path.isdir(parent) and not _is_fs_root(grandparent)): + _collect_package(grandparent, names) + + package = _apm_package_root(start) + if package: + _collect_package(package, names) + for dep_dir in _declared_dependency_dirs(package): + _collect_package(dep_dir, names) + + root = _authoring_root(start) + if root: + _collect_authoring_root(root, names) + else: + for base in _deployed_roots(start): + _collect_package(base, names) + return names + + +# --- Extraction ----------------------------------------------------------- +# False positives are the design constraint here, not recall. The rules: +# * A BARE target must be hyphenated AND sit in a boundary sentence (one +# carrying "do not"/"instead"/"rather than"/"not for"). Without the second +# condition, pc-run's "run pre-commit hooks" reads as a route to a +# non-existent `pre-commit` skill. +# * A BARE arrow target counts only in ADR-0020's compressed boundary form, +# `Not -> `. Without that, diagnose's process chain +# "fix -> regression-test" reads as a route to `regression-test`. +# * A backticked hyphenated token counts only inside a boundary sentence. +# Unconditionally, `pre-push` or `commit-msg` in a TRIGGER clause is a hard +# FAIL with no escape hatch. Gating it costs nothing (measured over this +# corpus: 54 targets before and after); DELETING it costs 7 real targets +# across three gitea skills, so it is gated, not removed. +# * SINGLE-WORD targets are deliberately NOT matchable bare — `research`, +# `triage`, `forge`, `prototype` and `tdd` are all real skill names and all +# ordinary English, so a bare-word rule would flag most of the corpus. A +# single-word target must be written `` `forge` `` or /forge to be seen. +# That is a known recall limitation, accepted over the false positives. +# Tool names (Read/Write/Edit) are excluded by the lowercase-only pattern; MCP +# tool names (issue_write) by its rejection of underscores; file names by its +# rejection of dots and slashes. +# +# ATTRIBUTIVE USE. The boundary-sentence gate above does NOT solve the +# `pre-commit` false positive, and the comment that claimed it did was wrong: +# "instead", "rather than", "do not" and "not for" are exactly the words a +# boundary clause uses, so the gate is open precisely where the risk is. All of +# these were hard dangling FAILs with no suppression: +# Use pre-commit hooks instead of ad-hoc scripts. +# Invoke the pull-request template instead of writing one by hand. +# Use conventional-commits formatting rather than free-form messages. +# Composes label-resolution logic instead of duplicating it. +# Do not use for X — run the `pre-push` hooks instead. +# What separates every one of them from a real route is grammar, not marking: +# the hyphenated token is a compound MODIFIER of the noun that follows it +# ("pre-commit hooks", "pull-request template"), where a route target is +# terminal — followed by punctuation, a conjunction, or a boundary word. So a +# target whose next token is an ordinary lowercase noun is CONFIRM-ONLY: it +# still resolves and still counts as a route when the name exists, but it can +# never raise a dangling error on its own. +# +# This is deliberately NOT the simpler "only marked targets may dangle" rule, +# which would have been wrong here: BOTH live true positives in this corpus are +# BARE — research's "(use neuledge-context)" and gitea-issues' "Composes +# gitea-labels-\n milestones", where the `>` fold yields "gitea-labels- +# milestones" and the trailing hyphen is what keeps it terminal. Marking is a +# poor proxy, so the follower token is the signal, and it is applied to +# backticked targets too. +# +# TERMINAL IS NOT ENOUGH — IN-SENTENCE CORROBORATION. The follower test clears +# `pre-push` in the example above only because that example happens to be +# followed by the noun "hooks". Move the same token into terminal position and +# it was a hard FAIL again, with no suppression mechanism anywhere in this gate: +# Do not use for running hooks — run `pre-commit` instead. +# Do not use for the commit message — see `commit-msg`. +# Do not use for type errors — run `type-check` first. +# Instead, use `semantic-release`. +# Do not use for the old flow — use the clean-up instead. +# Do not run end-to-end, run unit-tests. +# Every one of those is grammatically identical to a genuinely broken route: +# "route verb + hyphenated name + terminal" is also exactly how prose cites a +# tool, a hook, a file format or an English compound. Nothing local separates +# them, and the skills most exposed are the ones this contract sends authors +# back to rewrite first — pc-run, pc-author, vale-run, vale-config and the apm-* +# family are all ABOUT hyphenated tools. +# +# So the confidence to BLOCK a commit comes from the sentence, not the token: a +# prose-form target may raise a hard error only when its own sentence names at +# least one OTHER target that RESOLVES. A routing sentence proves itself by +# routing somewhere real; a lone unresolvable name proves nothing. That is not a +# rule fitted to the fixtures — it is the shape of both live true positives, +# which sit beside `write-docs` and `gitea-labels-milestones` respectively, and +# it changes this corpus's verdict by exactly nothing. +# +# An uncorroborated unresolvable target is NOT discarded: every caller reports +# it at its SUGGESTION tier, naming the target. The finding stays visible on +# every run; only the power to block a commit is withdrawn, which is the part +# that had no escape hatch. +# +# EXPLICIT ROUTE NOTATION is exempt from corroboration and always blocks: +# ADR-0020's compressed arrow (`Not -> `) and Claude Code's +# invocation form (`/`). Neither is ever how English cites a tool — nobody +# writes `-> pre-commit` or `/pre-commit` to mean the hook — so there is no +# ambiguity to resolve, and an author who wants a route checked unconditionally +# has two ways to say so. +# +# NAMESPACE: `plugin:skill` is live in this repo (native user-scope installs +# still resolve `gitea:gitea-prs`), so the patterns admit an optional +# `:` prefix and normalize_target() strips it before resolution. +NS = r"(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?" +NAME_ANY = NS + r"[a-z0-9]+(?:-[a-z0-9]+)*" +NAME_HYPH = NS + r"[a-z0-9]+(?:-[a-z0-9]+)+" +ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" + r"|that'?s|compose|composes|call|calls" + r"|routes?\s+to|delegates?\s+to|prefers?|switch(?:es)?\s+to" + r"|hands?\s+off\s+to)") +MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +# A boundary clause takes two shapes and BOTH count: the prose markers, and +# ADR-0020's compressed arrow form `Not -> `. +BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +SENTENCE_SPLIT = re.compile(u'(?<=[.!?])\\s+(?=[A-Z"“(])') + +# The token that may follow a route target without turning it into a compound +# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a +# head noun that names the artifact itself ("the git-workflow skill"). Anything +# else — `hooks`, `template`, `formatting`, `logic` — is attributive prose. +FOLLOWER = re.compile(r"[`\s]*([a-z][a-z0-9]*)") +FOLLOWER_OK = frozenset(""" +and or nor but for to when if unless while after before with from in on at by +of as than then instead rather directly first only always never also even +both either neither so because since per via plus alone here there this that +these those it its they them is are was were be been being has have had will +would can could should must may might does do did +skill skills agent agents plugin plugins command commands +""".split()) + + +def normalize_target(target): + """Comparison key: namespace stripped, lowercased. + + Extraction is case-insensitive (re.I) but the universe is built from + lowercase directory names, so `Git-Commits` at the start of a sentence + resolved to nothing until this normalization existed. + """ + return target.split(':')[-1].lower() + + +def has_boundary_clause(description): + return bool(BOUNDARY_MARKER.search(description) + or BOUNDARY_ARROW.search(description)) + + +def _first(match): + """(name, start, end) offsets for the first group that matched.""" + for index in range(1, (match.re.groups or 0) + 1): + if match.group(index): + return match.group(index), match.start(index), match.end(index) + return None, None, None + + +def _terminal(text, pos): + """True if the token at pos does not make the preceding name a modifier.""" + follower = FOLLOWER.match(text, pos) + return not follower or follower.group(1) in FOLLOWER_OK + + +def _notation(text, start, arrow): + """True if the name is written in route NOTATION rather than in prose. + + Two forms qualify: `/name` (Claude Code's invocation syntax, detected from + the character before the name) and `-> name` (ADR-0020's compressed boundary + form, passed in by the caller that matched the arrow). A backticked name + does NOT qualify — a code span is how a tool, a file and a skill are all + cited, so it carries no intent the follower test hasn't already read. + """ + return arrow or (start > 0 and text[start - 1] == '/') + + +def _add(out, text, name, start, end, strict=None, arrow=False): + if not name: + return + out.append((name, + _terminal(text, end) if strict is None else strict, + _notation(text, start, arrow))) + + +def _scan(text, route_re, cont_re, out): + for match in route_re.finditer(text): + name, start, end = _first(match) + if not name: + continue + _add(out, text, name, start, end) + # "use git-history or git-branches instead" / "use gitea-issues / + # gitea-prs" — keep consuming conjoined targets after the first. + pos = match.end() + while True: + cont = cont_re.match(text, pos) + if not cont: + break + _add(out, text, *_first(cont)) + pos = cont.end() + + +def _extract_sentence(sentence): + """[(name, may_dangle, notation)] for the routing targets in ONE sentence. + + Kept separate from _extract() because corroboration is scoped to a single + sentence: a target's evidence is what stands beside it, not what the rest of + the description happens to mention. + """ + out = [] + boundary = bool(BOUNDARY_MARKER.search(sentence)) + _scan(sentence, + ROUTE_ANY if boundary else ROUTE_MARKED, + CONT_ANY if boundary else CONT_MARKED, + out) + for match in ARROW_MARKED.finditer(sentence): + # `-> name` and `-> /name` are route notation, not prose: nothing + # reads as a compound modifier after an arrow, so no follower test. + _add(out, sentence, *_first(match), strict=True, arrow=True) + for match in ARROW_BOUNDARY.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1), + strict=True, arrow=True) + if boundary: + for match in BACKTICK.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1)) + return out + + +def _extract(description): + """[(name, may_dangle, notation)] for every routing target.""" + out = [] + for sentence in SENTENCE_SPLIT.split(description): + out.extend(_extract_sentence(sentence)) + return out + + +def boundary_targets(description): + """Every routing target, for reporting and for confirming a route.""" + return sorted({name for name, _, _ in _extract(description)}) + + +def unresolved_targets(description, known): + """Targets resolving to nothing, split into (blocking, reported). + + `blocking` earns a hard error; `reported` is SUGGESTION tier — named on + every run, never fatal. Three conditions gate the promotion, and all of them + are documented at length in the ATTRIBUTIVE USE and CORROBORATION notes + above: + + 1. the target must be terminal, not a compound modifier ("pre-commit + hooks" is prose about a tool, not a route), + 2. it must be written in route notation (`/name`, `-> name`), OR + 3. its own sentence must name another target that DOES resolve. + + Everything else is reported and left alone. `known` is the resolved + universe from known_targets(); passing an empty set is not meaningful — + callers check for that first and decline out loud instead. + """ + blocking, reported = set(), set() + for sentence in SENTENCE_SPLIT.split(description): + found = _extract_sentence(sentence) + resolved = {normalize_target(name) for name, _, _ in found + if normalize_target(name) in known} + for name, may_dangle, notation in found: + key = normalize_target(name) + if key in known or not may_dangle: + continue + if notation or (resolved - {key}): + blocking.add(name) + else: + reported.add(name) + return sorted(blocking), sorted(reported - blocking) + +# --- Frontmatter ---------------------------------------------------------- +# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading +# blank line, trailing whitespace after either `---`, or CRLF line endings all +# defeated the old `^---\n(.*?)\n---`, and the miss was SILENT: every ADR-0020 +# check was skipped and the file reported green (measured: a 550-character +# description with a 1,000-word body exited 0 behind a BOM). A file that cannot +# be measured must never report green, so every caller of these two ERRORs on a +# miss instead of moving on. +FRONTMATTER_RE = re.compile( + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + + +def strip_bom(text): + return text[1:] if text.startswith(u'') else text + + +class FrontmatterError(Exception): + pass + + +def description_value(fm_text): + """The description VALUE, with YAML folding resolved. + + PyYAML is a HARD requirement, preflighted in bash. The hand-rolled fallback + this replaced diverged from a real parser across the FAIL boundary — one + corpus description measured 270 characters parsed and 412 unparsed, and a + quoted `"description"` key or an explicit `description: null` returned empty + from it, silently skipping the description AND routing checks. A gate that + disagrees with itself depending on which reader ran is worse than no gate. + + This is the ONLY reader any of the three scripts may use to decide whether a + description is present. A line regex cannot: `description:` with no value + followed by `model: sonnet` lets `\\s*` cross the newline and captures the + NEXT key, which reads as a non-empty description, skips the "missing or + empty" failure, and then early-returns out of every ADR-0020 gate on the + genuinely empty folded value. That combination exited 0 with zero output on + a BLOCKING pre-push gate. + """ + try: + data = yaml.safe_load(fm_text) + except Exception as exc: + raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + if not isinstance(data, dict): + raise FrontmatterError('frontmatter is not a YAML mapping') + value = data.get('description') + if value is None: + return '' + if not isinstance(value, str): + value = str(value) + return re.sub(r'\s+', ' ', value).strip() + + +# --- Body-shape checks (skills only; agents have no references/ dir) ------- +# Deterministic and countable, so they are enforced here. Whether a given +# gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why +# both gotcha checks are SUGGESTION tier. A missing reference file is not a +# style opinion — it is a broken pointer — so that one is ERROR tier. +# +# Both read a FENCE-MASKED copy of the body. Scanning the raw body made a +# ```-fenced example a hard ERROR — and the skills most likely to carry one are +# skill-author and skill-audit, which DOCUMENT the references/ convention — and +# let a `## Gotchas` heading inside a fenced block stand in for the real +# section. Masking preserves every byte offset (content becomes spaces, +# newlines stay), so a span found in the mask slices the original. +GOTCHA_MAX_ENTRIES = 5 +GOTCHA_MAX_BODY_FRACTION = 0.25 +# The heading has to BE "Gotchas", not merely contain the word: `## Gotcha +# handling` and `## Why gotchas matter` are prose sections, and treating one as +# the Gotchas section measured a span that was never a gotcha list. +GOTCHA_HEADING = re.compile(r'^(#{1,6})[ \t]+(?:[^\n]*?[ \t])?gotchas?[ \t]*:?[ \t]*$', + re.I | re.M) +# Column 0 only. `^[ \t]{0,3}` counted a two-space-indented CHILD bullet as a +# top-level entry, so a five-entry section with sub-bullets reported nine. +GOTCHA_ENTRY = re.compile(r'^(?:[-*+]|\d+[.)])[ \t]+', re.M) +FENCE_OPEN = re.compile(r'^[ \t]{0,3}(`{3,}|~{3,})') +REFERENCE_POINTER = re.compile( + r'(?= len(fence) + and not stripped.strip()[len(marker):].strip()): + fence = None + return ''.join(out) + + +def gotcha_stats(body): + """(entry count, section word count) for the first Gotchas section, or None. + + The section runs to the next heading at the same level or shallower. + Entries are top-level list items; a section written as subheadings instead + of a list counts those. Headings and entries are read from the fence mask; + the word count is taken from the original slice, because fenced lines are + real body words and the fraction is measured against the whole body. + """ + masked = mask_fenced(body) + match = GOTCHA_HEADING.search(masked) + if not match: + return None + level = len(match.group(1)) + rest = masked[match.end():] + nxt = re.search(r'^#{1,%d}[ \t]+' % level, rest, re.M) + end = match.end() + (nxt.start() if nxt else len(rest)) + section = masked[match.end():end] + entries = len(GOTCHA_ENTRY.findall(section)) + if entries == 0 and level < 6: + entries = len(re.findall(r'^#{%d,6}[ \t]+' % (level + 1), section, re.M)) + return entries, len(body[match.end():end].split()) + + +def missing_reference_pointers(body, skill_dir): + """references/.md named in the body but absent from disk.""" + masked = mask_fenced(body) + missing = set() + for match in REFERENCE_POINTER.finditer(masked): + start = masked.rfind('\n', 0, match.start()) + 1 + end = masked.find('\n', match.end()) + if end < 0: + end = len(masked) + if REFERENCE_PAST.search(masked[start:end]): + continue + if REFERENCE_QUALIFIER.search(masked[start:match.start()]): + continue + if not os.path.isfile(os.path.join(skill_dir, 'references', match.group(1))): + missing.add('references/' + match.group(1)) + return sorted(missing) +# ===== END ADR-0020 SHARED BOUNDARY RESOLVER ===== + + def parse_frontmatter(content): - m = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) + m = FRONTMATTER_RE.match(strip_bom(content)) if not m: return None, content - return m.group(1), content[m.end():] + return m.group(1), strip_bom(content)[m.end():] def extract_field(fm, field): - m = re.search(rf'^{re.escape(field)}:\s*(.+)', fm, re.MULTILINE) + """The raw text after `field:` ON ITS OWN LINE, or None. + + The character class is `[^\\S\\r\\n]`, never `\\s`: under re.MULTILINE a + `\\s*` after the colon crosses the newline, so `description:` with no value + followed by `model: sonnet` captured `model: sonnet` as the description. + That made the value look present, skipped the "missing or empty" failure, + and then every ADR-0020 gate early-returned on the genuinely empty folded + value — a valueless description exited 0 with zero output on a BLOCKING + pre-push gate. This function is now used only for fields with no folding + semantics (name, tools); description goes through description_value(), the + shared resolver's YAML reader, which is the only thing that can see through + `>`, `null`, `''` and a quoted `"description"` key alike. + """ + m = re.search(rf'^{re.escape(field)}:[^\S\r\n]*(.+)', fm, re.MULTILINE) return m.group(1).strip() if m else None def get_frontmatter_keys(fm): @@ -139,65 +868,17 @@ def get_frontmatter_keys(fm): keys.add(m.group(1)) return keys -def normalize_scalar(value): - return re.sub(r'\s+', ' ', value).strip() - -def fold_description_fallback(fm_text): - """Resolve `description:` without PyYAML. - - Not a YAML parser — it recognises exactly the shapes agent frontmatter - uses: an inline scalar (optionally quoted, optionally continued on - following indented lines) and a `>`/`|` block scalar with optional - indentation and chomping indicators. - """ - lines = fm_text.splitlines() - for i, line in enumerate(lines): - m = re.match(r'^description:[ \t]*(.*)$', line) - if not m: - continue - head = m.group(1).strip() - block = bool(re.match(r'^[>|][0-9]*[-+]?$|^[>|][-+]?[0-9]*$', head)) - parts = [] if block else [head] - for nxt in lines[i + 1:]: - if not nxt.strip(): - parts.append('') - continue - if not re.match(r'^[ \t]', nxt): - break - parts.append(nxt.strip()) - value = ' '.join(parts) - if not block: - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in '"\'': - value = value[1:-1] - return value - return '' - -def description_value(fm): - """The description VALUE with YAML folding resolved. - - extract_field() reads one raw line, which is the right shape for the - presence and placeholder checks but the wrong one for a length gate: a - `>`-folded description measured off its first raw line is not the value the - host preloads. Parse instead of regexing the raw text. - """ +def agent_description(fm, local_fname): + """The folded description VALUE, or None if the frontmatter is not YAML.""" try: - import yaml - data = yaml.safe_load(fm) - if isinstance(data, dict): - value = data.get('description') - if isinstance(value, str): - return normalize_scalar(value) - if value is not None: - return normalize_scalar(str(value)) - return '' - except Exception: - pass - return normalize_scalar(fold_description_fallback(fm)) + return description_value(fm) + except FrontmatterError as exc: + fail(f"frontmatter is not valid YAML ({exc}) — the ADR-0020 description and " + f"boundary-target gates could not run — {local_fname}") + return None -def check_description_budget(fm, local_fname): +def check_description_budget(value, local_fname): """ADR-0020 description gates — identical for every scope.""" - value = description_value(fm) if not value: return dlen = len(value) @@ -213,6 +894,55 @@ def check_description_budget(fm, local_fname): f"what moves the corpus average; the FAIL tier only stops outliers " f"— {local_fname}") +def check_boundary(value, fpath, local_fname): + """ADR-0020 boundary clause + resolvable boundary targets. + + agent-author's SKILL.md states that an agent's boundary targets must + resolve, but until this ran no script checked it — the contract was + documented and unenforced. The resolution universe is derived from the + AGENT FILE's own location (the authoring root above it, its own apm + package, and that package's declared apm dependencies), never from this + script's path, and — when an authoring root exists — never from a deployed + .claude/ tree, so a fresh clone and a machine that has run `apm install` + return the same verdict. + """ + if not value: + return + # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether + # this particular agent warrants a boundary clause is judgment. All four + # agents in this corpus currently lack one. + if not has_boundary_clause(value): + suggest(f"description has no boundary clause — add the prose form (\"Do not use " + f"for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " + f"so the router knows where NOT to send this agent — {local_fname}") + targets = boundary_targets(value) + if not targets: + return + known = known_targets(os.path.dirname(os.path.abspath(fpath))) + if not known: + info(f"boundary-target resolution DID NOT RUN — no skill universe could be " + f"determined for this path (no authoring root above it, no apm package " + f"root, no declared apm dependencies, no deployed .claude/ or .agents/ " + f"tree). Unchecked target(s): {', '.join(targets)} — {local_fname}") + return + # blocking vs reported: a target only earns a FAIL when it is written in + # route notation or its own sentence corroborates it by naming another target + # that resolves. See the shared resolver's CORROBORATION note. + blocking, reported = unresolved_targets(value, known) + for target in blocking: + fail(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — a boundary clause naming a non-existent " + f"target sends the router nowhere — {local_fname}") + for target in reported: + suggest(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing " + f"else in that sentence resolves, so it is equally likely to be a tool, a " + f"file format or an English compound. If it IS a route, write it as " + f"`/{target}` or `-> {target}` and it will be checked properly — " + f"{local_fname}") + def extract_tools_list(fm): """Extract tool names from the tools frontmatter field (space or comma separated).""" val = extract_field(fm, 'tools') @@ -237,7 +967,10 @@ APM_TYPE_RE = re.compile(r"^type:\s*(['\"]?)(instructions|skill|hybrid|prompts)\ def find_apm_package_root(apm_yml_path): """Return True if apm_yml_path has a top-level type: line (i.e. is a package manifest, not a type:-less marketplace-only apm.yml).""" - with open(apm_yml_path) as f: + # errors='replace', not a hard failure: this only asks whether a `type:` + # line exists, and a stray undecodable byte elsewhere in someone else's + # apm.yml must not abort scope detection. + with open(apm_yml_path, encoding='utf-8', errors='replace') as f: for line in f: if APM_TYPE_RE.match(line): return True @@ -273,6 +1006,15 @@ def detect_scope(start_dir): conventional_root = os.path.dirname(os.path.dirname(original_start)) current = original_start while True: + # The filesystem root is never a candidate, the same guard the shared + # resolver's walk-up loops carry. Without it a file under a marker-less + # temp directory walked all the way to `/` and returned it as the scope + # root, which then reported `counterpart file not found: + # /.claude/agents/.md` — a path that names someone else's machine, + # not the user's project. When the walk runs out, the agent file's own + # directory (or its conventional root) is the honest answer. + if _is_fs_root(current): + return 'project', conventional_root if conventional_shape else original_start apm_yml = os.path.join(current, 'apm.yml') if os.path.isfile(apm_yml) and find_apm_package_root(apm_yml): return 'plugin', current @@ -315,12 +1057,19 @@ scope, scope_root = detect_scope(agent_dir) # --- Plugin/APM scope: single vendor-neutral file, no counterpart --- def check_apm_agent_file(fpath, allowlist, stem): local_fname = os.path.basename(fpath) - with open(fpath) as f: - content = f.read() + try: + content = read_text(fpath) + except EncodingError as exc: + fail(f"file is {exc}. Nothing could be measured, so this is a hard " + f"failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: - fail(f"no valid YAML frontmatter (---...---) — {local_fname}") + fail(f"no parseable YAML frontmatter block — expected a `---` line, the fields, " + f"then a closing `---` line (a BOM, leading blank lines, trailing spaces " + f"after either marker and CRLF endings are all tolerated). Nothing could be " + f"measured, so this is a hard failure, not a skip — {local_fname}") return # The apm-agent.md template embeds its authoring guidance as HTML @@ -358,13 +1107,21 @@ def check_apm_agent_file(fpath, allowlist, stem): fail(f"name '{name_val}' does not match filename stem '{stem}' — {local_fname}") # description — required, non-empty, no placeholder - desc_val = extract_field(fm, 'description') - if not desc_val: + # Presence is decided on the FOLDED value, never on a line regex. Deciding + # it on extract_field's raw capture is what let `description:` with no value + # pass this gate in total silence: the capture picked up the next key, so + # "missing or empty" never fired, and every ADR-0020 check below then + # early-returned on the empty folded value. Exit 0, zero output, no gate run. + folded = agent_description(fm, local_fname) + if folded is None: + pass # frontmatter is not valid YAML — agent_description already failed + elif not folded: fail(f"description field is missing or empty — {local_fname}") else: - if PLACEHOLDER_RE.search(desc_val): + if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(fm, local_fname) + check_description_budget(folded, local_fname) + check_boundary(folded, fpath, local_fname) # body — required, non-empty, no placeholder; same Copilot truncation risk # applies since this file compiles verbatim into a real Copilot file downstream. @@ -405,12 +1162,19 @@ else: # user def check_file(fpath, file_provider): local_fname = os.path.basename(fpath) - with open(fpath) as f: - content = f.read() + try: + content = read_text(fpath) + except EncodingError as exc: + fail(f"file is {exc}. Nothing could be measured, so this is a hard " + f"failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: - fail(f"no valid YAML frontmatter (---...---) — {local_fname}") + fail(f"no parseable YAML frontmatter block — expected a `---` line, the fields, " + f"then a closing `---` line (a BOM, leading blank lines, trailing spaces " + f"after either marker and CRLF endings are all tolerated). Nothing could be " + f"measured, so this is a hard failure, not a skip — {local_fname}") return # name — required for CC and Copilot CLI; optional for Copilot cloud/IDE agents @@ -432,13 +1196,21 @@ def check_file(fpath, file_provider): fail(f"name '{name_val}' is not kebab-case — {local_fname}") # description - desc_val = extract_field(fm, 'description') - if not desc_val: + # Presence is decided on the FOLDED value, never on a line regex. Deciding + # it on extract_field's raw capture is what let `description:` with no value + # pass this gate in total silence: the capture picked up the next key, so + # "missing or empty" never fired, and every ADR-0020 check below then + # early-returned on the empty folded value. Exit 0, zero output, no gate run. + folded = agent_description(fm, local_fname) + if folded is None: + pass # frontmatter is not valid YAML — agent_description already failed + elif not folded: fail(f"description field is missing or empty — {local_fname}") else: - if PLACEHOLDER_RE.search(desc_val): + if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(fm, local_fname) + check_description_budget(folded, local_fname) + check_boundary(folded, fpath, local_fname) # body if not body.strip(): diff --git a/plugins/kyberforge/.apm/skills/agent-audit/tests/validate.bats b/plugins/kyberforge/.apm/skills/agent-audit/tests/validate.bats index d827f5a..fbe1572 100644 --- a/plugins/kyberforge/.apm/skills/agent-audit/tests/validate.bats +++ b/plugins/kyberforge/.apm/skills/agent-audit/tests/validate.bats @@ -35,6 +35,24 @@ You are a test agent. When invoked, do the thing. EOF } + # Helper: a description of EXACTLY characters that carries a boundary + # clause and names no routing target. ADR-0020's missing-boundary-clause + # SUGGESTION fires on any description without one, so a fixture that omits it + # is never "otherwise clean" and a test refuting SUGGESTION would be asserting + # the boundary check's absence instead of the thing it names. The clause is + # paid for out of the measured budget rather than appended to it, because + # these tests measure the description LENGTH. "anything else" is not + # hyphenated, so no routing target comes with it. + desc_of_length() { + python3 - "$1" <<'PY' +import sys +n = int(sys.argv[1]) +prefix = 'Use when doing the thing. Do not use for anything else. ' +assert n >= len(prefix), 'requested description shorter than the boundary clause' +print(prefix + 'x' * (n - len(prefix))) +PY + } + # Helper: same shape as make_apm_agent, but the description is supplied # verbatim — used by the ADR-0020 description-budget tests. make_apm_agent_with_desc() { @@ -637,7 +655,7 @@ EOF @test "ADR-0020: agent description of exactly 250 chars raises no suggestion" { local root="$TMPDIR/pkg" - make_apm_agent_with_desc "$root" "my-agent" "$(python3 -c "print('x' * 250)")" + make_apm_agent_with_desc "$root" "my-agent" "$(desc_of_length 250)" run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" assert_success refute_output --partial "SUGGESTION" @@ -645,7 +663,7 @@ EOF @test "ADR-0020: agent description of 251 chars raises a SUGGESTION and still exits 0" { local root="$TMPDIR/pkg" - make_apm_agent_with_desc "$root" "my-agent" "$(python3 -c "print('x' * 251)")" + make_apm_agent_with_desc "$root" "my-agent" "$(desc_of_length 251)" run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" assert_success assert_output --partial "SUGGESTION" @@ -654,7 +672,7 @@ EOF @test "ADR-0020: agent description of exactly 400 chars is a SUGGESTION, not a FAIL" { local root="$TMPDIR/pkg" - make_apm_agent_with_desc "$root" "my-agent" "$(python3 -c "print('x' * 400)")" + make_apm_agent_with_desc "$root" "my-agent" "$(desc_of_length 400)" run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" assert_success assert_output --partial "SUGGESTION" @@ -662,7 +680,7 @@ EOF @test "ADR-0020: agent description of 401 chars FAILs and exits non-zero" { local root="$TMPDIR/pkg" - make_apm_agent_with_desc "$root" "my-agent" "$(python3 -c "print('x' * 401)")" + make_apm_agent_with_desc "$root" "my-agent" "$(desc_of_length 401)" run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" assert_failure assert_output --partial "description is 401 chars" @@ -732,10 +750,18 @@ EOF # body becomes the system prompt of a fresh context. ADR-0020 gates the # former at 900 words and explicitly declines to gate the latter. If a body # word gate is ever added here, it contradicts the ADR. + # + # The description carries a boundary clause so the ONLY thing this test can + # go red on is a body finding. Without one, the missing-boundary-clause + # SUGGESTION fires and the blanket `refute_output --partial "SUGGESTION"` + # below trips for a reason that has nothing to do with body length — which + # would look like the invariant breaking while proving nothing about it. + # AGENTS.md cites this test as the pin for that invariant, so it has to fail + # for one reason and one reason only. { echo "---" echo "name: my-agent" - echo "description: A valid agent description." + echo "description: A valid agent description. Do not use for anything else." echo "---" echo "" python3 -c "print(' '.join(['word'] * 1500))" @@ -743,7 +769,13 @@ EOF run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" assert_success refute_output --partial "FAIL" + # A 1,500-word body is 667% of the skill ceiling. Nothing may be said about + # it at any tier: not a FAIL, not a SUGGESTION, and not the word-count + # wording either tier would use if a gate were quietly added later. refute_output --partial "SUGGESTION" + refute_output --partial "1500 words" + refute_output --partial "900-word" + refute_output --partial "body is" } @test "a bare plugin.json with no apm.yml is no longer plugin scope — falls through to project scope" { diff --git a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh index 6995c5f..35c9733 100755 --- a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh @@ -28,12 +28,37 @@ if [[ $# -lt 1 ]]; then exit 1 fi +# PyYAML is a HARD dependency, not a nice-to-have. The description VALUE has to +# be measured after YAML folding is resolved, and the hand-rolled reader that +# used to stand in for PyYAML disagreed with it across the 400-character FAIL +# boundary — same description, two verdicts, depending on which reader ran. +# Refusing to start is the only honest option; the repo's jq / apm / vale +# dependencies are declared the same way. +# Check the interpreter separately from the library: `python3 -c` fails the same +# way whether python3 is missing or PyYAML is, and reporting the wrong missing +# dependency sends the reader to install the wrong thing. +if ! command -v python3 > /dev/null 2>&1; then + echo "Error: python3 is required but was not found on PATH." >&2 + echo " Why: skipping the ADR-0020 description, body and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: install python3 (pre-commit itself is a Python application, so it is almost certainly already present)." >&2 + exit 1 +fi + +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "Error: PyYAML is required but is not importable by python3." >&2 + echo " Why: skipping the ADR-0020 description, body and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi + python3 -u - "$1" <<'PYTHON' import sys import os import re import glob +import yaml + skill_dir = os.path.abspath(sys.argv[1]) skill_md = os.path.join(skill_dir, "SKILL.md") @@ -41,9 +66,6 @@ if not os.path.isfile(skill_md): print(f"Error: '{skill_md}' not found.", file=sys.stderr) sys.exit(1) -with open(skill_md) as f: - content = f.read() - failed = False suggestions = [] @@ -51,8 +73,12 @@ def ok(msg): print(f"PASS {msg}") def fail(msg): + # stderr, matching scripts/skill-size-check.sh's ERROR routing. All three + # scripts in the ADR-0020 family now agree: findings that fail the run go to + # stderr, everything advisory (PASS / SUGGESTION / INFO) goes to stdout. + # Both repo callers capture `2>&1`, so nothing a human reads moves. global failed - print(f"FAIL {msg}") + print(f"FAIL {msg}", file=sys.stderr) failed = True def suggest(msg): @@ -62,71 +88,724 @@ def suggest(msg): # rather than another silently-ignored warning (ADR-0013). suggestions.append(msg) +def info(msg): + # A check that DECLINED to run says so out loud, rather than passing + # silently. Silence is what let a whole gate family go missing unnoticed. + print(f"INFO {msg}") + + +# ===== BEGIN ADR-0020 SHARED BOUNDARY RESOLVER ===== +# ONE resolver, embedded VERBATIM in three scripts: +# scripts/skill-size-check.sh +# plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +# plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +# The block between these markers must stay byte-identical in all three. It is +# copied rather than imported because a cache-installed plugin's scripts cannot +# read files outside their own plugin directory, so there is no single file all +# three can share (same constraint that forces the ADR-0020 constants to be +# duplicated). Edit one copy, then paste it over the other two. +# +# Requires: glob, os, re, yaml (imported by the host script; PyYAML is a hard +# dependency, preflighted in bash before the interpreter starts). + +# --- Input ---------------------------------------------------------------- +# Every file this resolver's callers read goes through read_text(), which pins +# UTF-8 explicitly instead of inheriting locale.getpreferredencoding(). Under +# LC_ALL=C that inherited encoding is ASCII, so a perfectly ordinary em dash in +# a SKILL.md aborted the run with a bare UnicodeDecodeError traceback — loud, +# but pointing at the interpreter rather than at the file or the fix. A file +# that genuinely is not UTF-8 still fails; it just says so. + + +class EncodingError(Exception): + pass + + +def read_text(path): + """File contents as text, UTF-8, with a diagnostic instead of a traceback.""" + try: + with open(path, encoding='utf-8') as fh: + return fh.read() + except UnicodeDecodeError as exc: + raise EncodingError( + "not valid UTF-8 (%s at byte %d) — re-save the file as UTF-8; " + "this gate does not guess at other encodings" + % (exc.reason, exc.start)) + + +# --- Universe ------------------------------------------------------------ +# The set of names a boundary clause may resolve against is derived from an +# AUTHORING ROOT found by walking up FROM THE TARGET FILE. It is NEVER derived +# from this script's own location: deriving it from ${BASH_SOURCE} leaked +# holocron's 39-skill universe into every consumer repo that ran this hook +# through pre-commit, so a consumer skill routing to `skill-audit` resolved +# against a plugin it had never installed. +# +# An authoring root is the nearest ancestor holding plugins/*/.apm/skills/ or +# plugins/*/.apm/agents/ (a plugin monorepo), falling back to the nearest +# ancestor holding .git. When one is found the universe is: +# 1. every skill and agent under /plugins/*/ — sibling plugins resolve, +# which is what a monorepo means, +# 2. the target's own apm package, +# 3. the packages that package DECLARES in apm.yml dependencies.apm. +# Deployed .claude/ and .agents/ trees are deliberately NOT consulted in that +# case. They are `apm install` output, gitignored, and present only on a machine +# that has run it: four cross-plugin targets in this repo (gitea-branches -> +# git-branches, gitea-branches -> git-history, gitea-issues -> git-branches, +# gitea-workflow -> git-workflow) resolved through .claude/skills/ alone, so the +# same commit measured 2 dangling targets on a developer machine and 6 on a +# fresh clone. A gate shipping hot with no baseline cannot give two answers. +# +# Deployed trees are used only when NO authoring root exists — the consumer +# case, where the file being checked lives in or beside a deployed tree and +# there is no monorepo to read. + + +def _is_fs_root(path): + return os.path.dirname(path) == path + + +def _collect_package(pkg_dir, names): + """Add every skill/agent name a package directory exposes, any layout.""" + for sub in ('.apm/skills/*/', 'skills/*/'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + names.add(os.path.basename(path.rstrip('/')).lower()) + for sub in ('.apm/agents/*.md', 'agents/*.md'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + base = os.path.basename(path) + if base.endswith('.agent.md'): + base = base[:-len('.agent.md')] + else: + base = base[:-len('.md')] + names.add(base.lower()) + + +def _apm_package_root(start_dir): + """Nearest ancestor that is an apm package root (apm.yml or .apm/). + + The filesystem root is never a candidate: a stray /.apm/skills/ — a + scaffolding test's leftover, say, and one really does exist on at least one + machine here — would otherwise become the package root of every path on it. + Capped at ten levels so a pathological path can't become a filesystem + crawl; that covers every real layout by a wide margin. + """ + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + return None + if (os.path.isfile(os.path.join(current, 'apm.yml')) + or os.path.isdir(os.path.join(current, '.apm'))): + return current + current = os.path.dirname(current) + return None + + +def _authoring_root(start_dir): + """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + + Two passes, not one interleaved walk: a nested .git (a submodule, a + worktree of a sub-package) must not win over a real plugins/ root further + up. Both passes stop before the filesystem root for the same reason + _apm_package_root does. + """ + for probe in ( + lambda d: bool(glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))): + current = os.path.abspath(start_dir) + for _ in range(12): + if _is_fs_root(current): + break + if probe(current): + return current + current = os.path.dirname(current) + return None + + +def _collect_authoring_root(root, names): + """Every plugin in the monorepo contributes its names.""" + for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + if os.path.isdir(pkg): + _collect_package(pkg, names) + + +def _declared_dependency_dirs(pkg_dir): + """Directories of the apm packages pkg_dir's manifest DECLARES. + + Reads dependencies.apm and resolves each entry to a directory on disk: + a monorepo-relative `path:` (against the package root and the nearest + ancestor manifest, which is the monorepo root) or an installed + apm_modules//. Entries that resolve to nothing are skipped — an + undeployed dependency contributes no names rather than an error. + """ + manifest = os.path.join(pkg_dir, 'apm.yml') + if not os.path.isfile(manifest): + return [] + try: + data = yaml.safe_load(read_text(manifest)) or {} + except Exception: + return [] + if not isinstance(data, dict): + return [] + deps = data.get('dependencies') + deps = deps.get('apm') if isinstance(deps, dict) else None + if not isinstance(deps, list): + return [] + + roots = [pkg_dir] + ancestor = os.path.dirname(os.path.abspath(pkg_dir)) + for _ in range(10): + if _is_fs_root(ancestor): + break + if os.path.isfile(os.path.join(ancestor, 'apm.yml')): + roots.append(ancestor) + break + ancestor = os.path.dirname(ancestor) + + found = [] + for entry in deps: + candidates = [] + if isinstance(entry, dict): + rel = entry.get('path') + name = entry.get('name') + if not name and rel: + name = os.path.basename(str(rel).rstrip('/')) + if rel: + candidates.extend(os.path.join(r, str(rel)) for r in roots) + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', str(name))) + elif isinstance(entry, str): + name = re.split(r'[#@]', entry)[0].strip().rstrip('/').split('/')[-1] + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', name)) + candidates.extend(os.path.join(r, 'plugins', name) for r in roots) + for candidate in candidates: + if os.path.isdir(candidate): + found.append(candidate) + return found + + +def _deployed_roots(start_dir): + """.claude/ and .agents/ trees above start_dir — what a host really sees. + + Consulted ONLY when no authoring root exists; see the section header. The + filesystem root is skipped for the same reason _apm_package_root skips it: + a stray /.claude/skills/ must not join every path's universe. + """ + found = [] + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + break + for name in ('.claude', '.agents'): + base = os.path.join(current, name) + if os.path.isdir(base): + found.append(base) + current = os.path.dirname(current) + return found + + +def known_targets(start_dir): + """Every skill/agent name a boundary clause in start_dir may name.""" + names = set() + start = os.path.abspath(start_dir) + + # Siblings: a cache-installed plugin and a deployed .claude/skills/ tree + # both put peers one level up, with no plugins/ directory above them. The + # grandparent is guarded against the filesystem root exactly like the two + # walk-up loops above — for a start dir of /skills/ the grandparent is + # `/`, and collecting there picks up this machine's stray /.apm/skills/. + parent = os.path.dirname(start) + grandparent = os.path.dirname(parent) + if (os.path.basename(parent) in ('skills', 'agents') + and os.path.isdir(parent) and not _is_fs_root(grandparent)): + _collect_package(grandparent, names) + + package = _apm_package_root(start) + if package: + _collect_package(package, names) + for dep_dir in _declared_dependency_dirs(package): + _collect_package(dep_dir, names) + + root = _authoring_root(start) + if root: + _collect_authoring_root(root, names) + else: + for base in _deployed_roots(start): + _collect_package(base, names) + return names + + +# --- Extraction ----------------------------------------------------------- +# False positives are the design constraint here, not recall. The rules: +# * A BARE target must be hyphenated AND sit in a boundary sentence (one +# carrying "do not"/"instead"/"rather than"/"not for"). Without the second +# condition, pc-run's "run pre-commit hooks" reads as a route to a +# non-existent `pre-commit` skill. +# * A BARE arrow target counts only in ADR-0020's compressed boundary form, +# `Not -> `. Without that, diagnose's process chain +# "fix -> regression-test" reads as a route to `regression-test`. +# * A backticked hyphenated token counts only inside a boundary sentence. +# Unconditionally, `pre-push` or `commit-msg` in a TRIGGER clause is a hard +# FAIL with no escape hatch. Gating it costs nothing (measured over this +# corpus: 54 targets before and after); DELETING it costs 7 real targets +# across three gitea skills, so it is gated, not removed. +# * SINGLE-WORD targets are deliberately NOT matchable bare — `research`, +# `triage`, `forge`, `prototype` and `tdd` are all real skill names and all +# ordinary English, so a bare-word rule would flag most of the corpus. A +# single-word target must be written `` `forge` `` or /forge to be seen. +# That is a known recall limitation, accepted over the false positives. +# Tool names (Read/Write/Edit) are excluded by the lowercase-only pattern; MCP +# tool names (issue_write) by its rejection of underscores; file names by its +# rejection of dots and slashes. +# +# ATTRIBUTIVE USE. The boundary-sentence gate above does NOT solve the +# `pre-commit` false positive, and the comment that claimed it did was wrong: +# "instead", "rather than", "do not" and "not for" are exactly the words a +# boundary clause uses, so the gate is open precisely where the risk is. All of +# these were hard dangling FAILs with no suppression: +# Use pre-commit hooks instead of ad-hoc scripts. +# Invoke the pull-request template instead of writing one by hand. +# Use conventional-commits formatting rather than free-form messages. +# Composes label-resolution logic instead of duplicating it. +# Do not use for X — run the `pre-push` hooks instead. +# What separates every one of them from a real route is grammar, not marking: +# the hyphenated token is a compound MODIFIER of the noun that follows it +# ("pre-commit hooks", "pull-request template"), where a route target is +# terminal — followed by punctuation, a conjunction, or a boundary word. So a +# target whose next token is an ordinary lowercase noun is CONFIRM-ONLY: it +# still resolves and still counts as a route when the name exists, but it can +# never raise a dangling error on its own. +# +# This is deliberately NOT the simpler "only marked targets may dangle" rule, +# which would have been wrong here: BOTH live true positives in this corpus are +# BARE — research's "(use neuledge-context)" and gitea-issues' "Composes +# gitea-labels-\n milestones", where the `>` fold yields "gitea-labels- +# milestones" and the trailing hyphen is what keeps it terminal. Marking is a +# poor proxy, so the follower token is the signal, and it is applied to +# backticked targets too. +# +# TERMINAL IS NOT ENOUGH — IN-SENTENCE CORROBORATION. The follower test clears +# `pre-push` in the example above only because that example happens to be +# followed by the noun "hooks". Move the same token into terminal position and +# it was a hard FAIL again, with no suppression mechanism anywhere in this gate: +# Do not use for running hooks — run `pre-commit` instead. +# Do not use for the commit message — see `commit-msg`. +# Do not use for type errors — run `type-check` first. +# Instead, use `semantic-release`. +# Do not use for the old flow — use the clean-up instead. +# Do not run end-to-end, run unit-tests. +# Every one of those is grammatically identical to a genuinely broken route: +# "route verb + hyphenated name + terminal" is also exactly how prose cites a +# tool, a hook, a file format or an English compound. Nothing local separates +# them, and the skills most exposed are the ones this contract sends authors +# back to rewrite first — pc-run, pc-author, vale-run, vale-config and the apm-* +# family are all ABOUT hyphenated tools. +# +# So the confidence to BLOCK a commit comes from the sentence, not the token: a +# prose-form target may raise a hard error only when its own sentence names at +# least one OTHER target that RESOLVES. A routing sentence proves itself by +# routing somewhere real; a lone unresolvable name proves nothing. That is not a +# rule fitted to the fixtures — it is the shape of both live true positives, +# which sit beside `write-docs` and `gitea-labels-milestones` respectively, and +# it changes this corpus's verdict by exactly nothing. +# +# An uncorroborated unresolvable target is NOT discarded: every caller reports +# it at its SUGGESTION tier, naming the target. The finding stays visible on +# every run; only the power to block a commit is withdrawn, which is the part +# that had no escape hatch. +# +# EXPLICIT ROUTE NOTATION is exempt from corroboration and always blocks: +# ADR-0020's compressed arrow (`Not -> `) and Claude Code's +# invocation form (`/`). Neither is ever how English cites a tool — nobody +# writes `-> pre-commit` or `/pre-commit` to mean the hook — so there is no +# ambiguity to resolve, and an author who wants a route checked unconditionally +# has two ways to say so. +# +# NAMESPACE: `plugin:skill` is live in this repo (native user-scope installs +# still resolve `gitea:gitea-prs`), so the patterns admit an optional +# `:` prefix and normalize_target() strips it before resolution. +NS = r"(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?" +NAME_ANY = NS + r"[a-z0-9]+(?:-[a-z0-9]+)*" +NAME_HYPH = NS + r"[a-z0-9]+(?:-[a-z0-9]+)+" +ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" + r"|that'?s|compose|composes|call|calls" + r"|routes?\s+to|delegates?\s+to|prefers?|switch(?:es)?\s+to" + r"|hands?\s+off\s+to)") +MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +# A boundary clause takes two shapes and BOTH count: the prose markers, and +# ADR-0020's compressed arrow form `Not -> `. +BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +SENTENCE_SPLIT = re.compile(u'(?<=[.!?])\\s+(?=[A-Z"“(])') + +# The token that may follow a route target without turning it into a compound +# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a +# head noun that names the artifact itself ("the git-workflow skill"). Anything +# else — `hooks`, `template`, `formatting`, `logic` — is attributive prose. +FOLLOWER = re.compile(r"[`\s]*([a-z][a-z0-9]*)") +FOLLOWER_OK = frozenset(""" +and or nor but for to when if unless while after before with from in on at by +of as than then instead rather directly first only always never also even +both either neither so because since per via plus alone here there this that +these those it its they them is are was were be been being has have had will +would can could should must may might does do did +skill skills agent agents plugin plugins command commands +""".split()) + + +def normalize_target(target): + """Comparison key: namespace stripped, lowercased. + + Extraction is case-insensitive (re.I) but the universe is built from + lowercase directory names, so `Git-Commits` at the start of a sentence + resolved to nothing until this normalization existed. + """ + return target.split(':')[-1].lower() + + +def has_boundary_clause(description): + return bool(BOUNDARY_MARKER.search(description) + or BOUNDARY_ARROW.search(description)) + + +def _first(match): + """(name, start, end) offsets for the first group that matched.""" + for index in range(1, (match.re.groups or 0) + 1): + if match.group(index): + return match.group(index), match.start(index), match.end(index) + return None, None, None + + +def _terminal(text, pos): + """True if the token at pos does not make the preceding name a modifier.""" + follower = FOLLOWER.match(text, pos) + return not follower or follower.group(1) in FOLLOWER_OK + + +def _notation(text, start, arrow): + """True if the name is written in route NOTATION rather than in prose. + + Two forms qualify: `/name` (Claude Code's invocation syntax, detected from + the character before the name) and `-> name` (ADR-0020's compressed boundary + form, passed in by the caller that matched the arrow). A backticked name + does NOT qualify — a code span is how a tool, a file and a skill are all + cited, so it carries no intent the follower test hasn't already read. + """ + return arrow or (start > 0 and text[start - 1] == '/') + + +def _add(out, text, name, start, end, strict=None, arrow=False): + if not name: + return + out.append((name, + _terminal(text, end) if strict is None else strict, + _notation(text, start, arrow))) + + +def _scan(text, route_re, cont_re, out): + for match in route_re.finditer(text): + name, start, end = _first(match) + if not name: + continue + _add(out, text, name, start, end) + # "use git-history or git-branches instead" / "use gitea-issues / + # gitea-prs" — keep consuming conjoined targets after the first. + pos = match.end() + while True: + cont = cont_re.match(text, pos) + if not cont: + break + _add(out, text, *_first(cont)) + pos = cont.end() + + +def _extract_sentence(sentence): + """[(name, may_dangle, notation)] for the routing targets in ONE sentence. + + Kept separate from _extract() because corroboration is scoped to a single + sentence: a target's evidence is what stands beside it, not what the rest of + the description happens to mention. + """ + out = [] + boundary = bool(BOUNDARY_MARKER.search(sentence)) + _scan(sentence, + ROUTE_ANY if boundary else ROUTE_MARKED, + CONT_ANY if boundary else CONT_MARKED, + out) + for match in ARROW_MARKED.finditer(sentence): + # `-> name` and `-> /name` are route notation, not prose: nothing + # reads as a compound modifier after an arrow, so no follower test. + _add(out, sentence, *_first(match), strict=True, arrow=True) + for match in ARROW_BOUNDARY.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1), + strict=True, arrow=True) + if boundary: + for match in BACKTICK.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1)) + return out + + +def _extract(description): + """[(name, may_dangle, notation)] for every routing target.""" + out = [] + for sentence in SENTENCE_SPLIT.split(description): + out.extend(_extract_sentence(sentence)) + return out + + +def boundary_targets(description): + """Every routing target, for reporting and for confirming a route.""" + return sorted({name for name, _, _ in _extract(description)}) + + +def unresolved_targets(description, known): + """Targets resolving to nothing, split into (blocking, reported). + + `blocking` earns a hard error; `reported` is SUGGESTION tier — named on + every run, never fatal. Three conditions gate the promotion, and all of them + are documented at length in the ATTRIBUTIVE USE and CORROBORATION notes + above: + + 1. the target must be terminal, not a compound modifier ("pre-commit + hooks" is prose about a tool, not a route), + 2. it must be written in route notation (`/name`, `-> name`), OR + 3. its own sentence must name another target that DOES resolve. + + Everything else is reported and left alone. `known` is the resolved + universe from known_targets(); passing an empty set is not meaningful — + callers check for that first and decline out loud instead. + """ + blocking, reported = set(), set() + for sentence in SENTENCE_SPLIT.split(description): + found = _extract_sentence(sentence) + resolved = {normalize_target(name) for name, _, _ in found + if normalize_target(name) in known} + for name, may_dangle, notation in found: + key = normalize_target(name) + if key in known or not may_dangle: + continue + if notation or (resolved - {key}): + blocking.add(name) + else: + reported.add(name) + return sorted(blocking), sorted(reported - blocking) + +# --- Frontmatter ---------------------------------------------------------- +# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading +# blank line, trailing whitespace after either `---`, or CRLF line endings all +# defeated the old `^---\n(.*?)\n---`, and the miss was SILENT: every ADR-0020 +# check was skipped and the file reported green (measured: a 550-character +# description with a 1,000-word body exited 0 behind a BOM). A file that cannot +# be measured must never report green, so every caller of these two ERRORs on a +# miss instead of moving on. +FRONTMATTER_RE = re.compile( + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + + +def strip_bom(text): + return text[1:] if text.startswith(u'') else text + + +class FrontmatterError(Exception): + pass + + +def description_value(fm_text): + """The description VALUE, with YAML folding resolved. + + PyYAML is a HARD requirement, preflighted in bash. The hand-rolled fallback + this replaced diverged from a real parser across the FAIL boundary — one + corpus description measured 270 characters parsed and 412 unparsed, and a + quoted `"description"` key or an explicit `description: null` returned empty + from it, silently skipping the description AND routing checks. A gate that + disagrees with itself depending on which reader ran is worse than no gate. + + This is the ONLY reader any of the three scripts may use to decide whether a + description is present. A line regex cannot: `description:` with no value + followed by `model: sonnet` lets `\\s*` cross the newline and captures the + NEXT key, which reads as a non-empty description, skips the "missing or + empty" failure, and then early-returns out of every ADR-0020 gate on the + genuinely empty folded value. That combination exited 0 with zero output on + a BLOCKING pre-push gate. + """ + try: + data = yaml.safe_load(fm_text) + except Exception as exc: + raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + if not isinstance(data, dict): + raise FrontmatterError('frontmatter is not a YAML mapping') + value = data.get('description') + if value is None: + return '' + if not isinstance(value, str): + value = str(value) + return re.sub(r'\s+', ' ', value).strip() + + +# --- Body-shape checks (skills only; agents have no references/ dir) ------- +# Deterministic and countable, so they are enforced here. Whether a given +# gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why +# both gotcha checks are SUGGESTION tier. A missing reference file is not a +# style opinion — it is a broken pointer — so that one is ERROR tier. +# +# Both read a FENCE-MASKED copy of the body. Scanning the raw body made a +# ```-fenced example a hard ERROR — and the skills most likely to carry one are +# skill-author and skill-audit, which DOCUMENT the references/ convention — and +# let a `## Gotchas` heading inside a fenced block stand in for the real +# section. Masking preserves every byte offset (content becomes spaces, +# newlines stay), so a span found in the mask slices the original. +GOTCHA_MAX_ENTRIES = 5 +GOTCHA_MAX_BODY_FRACTION = 0.25 +# The heading has to BE "Gotchas", not merely contain the word: `## Gotcha +# handling` and `## Why gotchas matter` are prose sections, and treating one as +# the Gotchas section measured a span that was never a gotcha list. +GOTCHA_HEADING = re.compile(r'^(#{1,6})[ \t]+(?:[^\n]*?[ \t])?gotchas?[ \t]*:?[ \t]*$', + re.I | re.M) +# Column 0 only. `^[ \t]{0,3}` counted a two-space-indented CHILD bullet as a +# top-level entry, so a five-entry section with sub-bullets reported nine. +GOTCHA_ENTRY = re.compile(r'^(?:[-*+]|\d+[.)])[ \t]+', re.M) +FENCE_OPEN = re.compile(r'^[ \t]{0,3}(`{3,}|~{3,})') +REFERENCE_POINTER = re.compile( + r'(?= len(fence) + and not stripped.strip()[len(marker):].strip()): + fence = None + return ''.join(out) + + +def gotcha_stats(body): + """(entry count, section word count) for the first Gotchas section, or None. + + The section runs to the next heading at the same level or shallower. + Entries are top-level list items; a section written as subheadings instead + of a list counts those. Headings and entries are read from the fence mask; + the word count is taken from the original slice, because fenced lines are + real body words and the fraction is measured against the whole body. + """ + masked = mask_fenced(body) + match = GOTCHA_HEADING.search(masked) + if not match: + return None + level = len(match.group(1)) + rest = masked[match.end():] + nxt = re.search(r'^#{1,%d}[ \t]+' % level, rest, re.M) + end = match.end() + (nxt.start() if nxt else len(rest)) + section = masked[match.end():end] + entries = len(GOTCHA_ENTRY.findall(section)) + if entries == 0 and level < 6: + entries = len(re.findall(r'^#{%d,6}[ \t]+' % (level + 1), section, re.M)) + return entries, len(body[match.end():end].split()) + + +def missing_reference_pointers(body, skill_dir): + """references/.md named in the body but absent from disk.""" + masked = mask_fenced(body) + missing = set() + for match in REFERENCE_POINTER.finditer(masked): + start = masked.rfind('\n', 0, match.start()) + 1 + end = masked.find('\n', match.end()) + if end < 0: + end = len(masked) + if REFERENCE_PAST.search(masked[start:end]): + continue + if REFERENCE_QUALIFIER.search(masked[start:match.start()]): + continue + if not os.path.isfile(os.path.join(skill_dir, 'references', match.group(1))): + missing.add('references/' + match.group(1)) + return sorted(missing) +# ===== END ADR-0020 SHARED BOUNDARY RESOLVER ===== + + +# A leading BOM is stripped before anything is parsed or counted. It changes +# neither count below — it is not a line separator and str.split() does not +# treat it as whitespace — but it did defeat the frontmatter match. +try: + content = strip_bom(read_text(skill_md)) +except EncodingError as exc: + fail(f"SKILL.md is {exc}. Nothing downstream can be measured, so this is a " + f"hard failure, not a skip") + print("One or more checks failed.") + sys.exit(1) + # --- Parse frontmatter --- -fm_match = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) +fm_match = FRONTMATTER_RE.match(content) if not fm_match: - fail("No valid YAML frontmatter block found (expected ---...---)") + fail("No parseable YAML frontmatter block found. Expected a `---` line, the " + "fields, then a closing `---` line (a BOM, leading blank lines, trailing " + "spaces after either marker and CRLF endings are all tolerated). Nothing " + "downstream can be measured, so this is a hard failure, not a skip") + print("One or more checks failed.") sys.exit(1) fm = fm_match.group(1) body_start = fm_match.end() -# Extract name -name_m = re.search(r'^name:\s*(\S+)', fm, re.MULTILINE) +# Extract name. The character class is `[ \t]`, never `\s`: under re.MULTILINE +# a `\s*` after the colon crosses the newline, so a valueless `name:` followed +# by `description: ...` captured the NEXT KEY as the name and reported a +# mismatch instead of an absence. Same class of bug as the `description:` one +# the shared resolver's description_value() docstring records. +name_m = re.search(r'^name:[ \t]*(\S+)', fm, re.MULTILINE) name = name_m.group(1).strip('"\'') if name_m else "" # Extract description — the VALUE, with YAML folding resolved. Most of this # corpus writes descriptions as `>`-folded block scalars, so the raw lines # carry indentation and newlines that are not part of the value: every length -# measurement below is wrong unless the scalar is folded first. PyYAML is used -# when importable (it is a real parser); the fallback recognises exactly the -# shapes this corpus uses — an inline scalar, optionally quoted and optionally -# continued on following indented lines, and a `>`/`|` block scalar with -# optional indentation and chomping indicators. - -def normalize(value): - return re.sub(r'\s+', ' ', value).strip() - -def fold_description_fallback(fm_text): - lines = fm_text.splitlines() - for i, line in enumerate(lines): - m = re.match(r'^description:[ \t]*(.*)$', line) - if not m: - continue - head = m.group(1).strip() - block = bool(re.match(r'^[>|][0-9]*[-+]?$|^[>|][-+]?[0-9]*$', head)) - parts = [] if block else [head] - for nxt in lines[i + 1:]: - if not nxt.strip(): - parts.append('') - continue - if not re.match(r'^[ \t]', nxt): - break - parts.append(nxt.strip()) - value = ' '.join(parts) - if not block: - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in '"\'': - value = value[1:-1] - return value - return '' - -def extract_description(fm_text): - try: - import yaml - data = yaml.safe_load(fm_text) - if isinstance(data, dict): - value = data.get('description') - if isinstance(value, str): - return normalize(value) - if value is not None: - return normalize(str(value)) - return '' - except Exception: - pass - return normalize(fold_description_fallback(fm_text)) - -desc = extract_description(fm) +# measurement below is wrong unless the scalar is folded first. +try: + desc = description_value(fm) +except FrontmatterError as exc: + fail(f"frontmatter is not valid YAML ({exc}). Nothing downstream can be " + f"measured, so this is a hard failure, not a skip") + print("One or more checks failed.") + sys.exit(1) dir_name = os.path.basename(skill_dir) @@ -155,13 +834,13 @@ if name: # name format if name: if re.match(r'^[a-z0-9]+(-[a-z0-9]+)*$', name): - ok(f"name format valid (kebab-case)") + ok("name format valid (kebab-case)") else: fail(f"name '{name}' is invalid — use lowercase letters, numbers, and hyphens only; no leading, trailing, or consecutive hyphens") # description present if desc: - ok(f"description present") + ok("description present") else: fail("description field is missing or empty") @@ -270,135 +949,85 @@ elif body_word_count > BODY_SUGGEST_WORDS: else: ok(f"SKILL.md body word count {body_word_count} (ADR-0020 target: {BODY_SUGGEST_WORDS})") +# --- Reference pointers must exist ----------------------------------------- +# FAIL, not SUGGESTION: a dispatch table naming a references/ file that is not +# on disk is a hard break, and until this check existed nothing in the +# gate/audit/vale stack noticed it — all three exited 0. +missing_refs = missing_reference_pointers(body, skill_dir) +for ref in missing_refs: + fail(f"SKILL.md body points at {ref}, which does not exist on disk — a dispatch " + f"table or \"read X\" trigger naming a missing file sends the agent nowhere") +if not missing_refs: + ok("all referenced references/ files exist") + +# --- Gotchas discipline ----------------------------------------------------- +# SUGGESTION on both counts: the measurement is deterministic, but whether a +# given gotcha earns its place in the body is the auditor's judgment. +gotchas = gotcha_stats(body) +if gotchas is not None: + gotcha_entries, gotcha_words = gotchas + if gotcha_entries > GOTCHA_MAX_ENTRIES: + suggest(f"Gotchas section has {gotcha_entries} entries — over the " + f"{GOTCHA_MAX_ENTRIES}-entry guideline. A list that long is usually a " + f"missing references/ file or a design problem written up as a warning") + if body_word_count and gotcha_words > body_word_count * GOTCHA_MAX_BODY_FRACTION: + suggest(f"Gotchas section is {gotcha_words} of {body_word_count} body words " + f"({round(100.0 * gotcha_words / body_word_count)}%) — over the " + f"{round(100.0 * GOTCHA_MAX_BODY_FRACTION)}% guideline. Move the durable " + f"parts to references/ and keep the section for live traps") + +# --- ADR-0020: boundary clause present ------------------------------------- +# SUGGESTION, not FAIL: detecting the absence is deterministic, but whether +# this particular skill warrants a boundary clause is judgment. Both accepted +# shapes count — the prose markers and the compressed `Not -> `. +if desc: + if has_boundary_clause(desc): + ok("description has a boundary clause") + else: + suggest("description has no boundary clause — add the prose form (\"Do not use " + "for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " + "so the router knows where NOT to send this skill") + # --- ADR-0020: resolvable boundary targets --------------------------------- -# A boundary clause names another skill — or an agent, which is an equally -# valid routing target (git-workflow routes to the git-orchestrate agent). Every -# named target is resolved against the AUTHORING SOURCE, plugins/*/.apm/skills/ -# and plugins/*/.apm/agents/, so the check works offline and before an -# `apm install` has deployed anything into .claude/skills/. -# -# False positives are the design constraint here, not recall. Two rules do the -# work: -# * A BARE hyphenated word is read as a routing target only inside a boundary -# sentence (one carrying "do not"/"instead"/"rather than"/"not for"). -# Without that, pc-run's "run pre-commit hooks" reads as a route to a -# non-existent `pre-commit` skill. -# * A BARE arrow target counts only in ADR-0020's compressed boundary form, -# `Not -> `. Without that, diagnose's process chain -# "fix -> regression-test" reads as a route to `regression-test`. -# Backticked and /slash-command targets are unambiguous and always count. Tool -# names (Read, Write, Edit) are excluded by the lowercase-only name pattern; -# MCP tool names (issue_write, pull_request_write) by its rejection of -# underscores; file names by its rejection of dots and slashes. - -NAME_ANY = r"[a-z0-9]+(?:-[a-z0-9]+)*" -NAME_HYPH = r"[a-z0-9]+(?:-[a-z0-9]+)+" -ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" - r"|that'?s|compose|composes|call|calls)") -MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) -BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -SENTENCE_SPLIT = re.compile('(?<=[.!?])\\s+(?=[A-Z"“(])') - -def _first_group(groups): - for g in groups: - if g: - return g - return None - -def _scan_routes(text, route_re, cont_re, out): - for m in route_re.finditer(text): - target = _first_group(m.groups()) - if not target: - continue - out.append(target) - # Conjoined targets: "use git-history or git-branches instead", - # "use gitea-issues / gitea-prs". - pos = m.end() - while True: - cm = cont_re.match(text, pos) - if not cm: - break - nxt = _first_group(cm.groups()) - if nxt: - out.append(nxt) - pos = cm.end() - -def boundary_targets(description): - out = [] - for sentence in SENTENCE_SPLIT.split(description): - boundary = bool(BOUNDARY_MARKER.search(sentence)) - _scan_routes(sentence, - ROUTE_ANY if boundary else ROUTE_MARKED, - CONT_ANY if boundary else CONT_MARKED, - out) - for m in ARROW_MARKED.finditer(sentence): - target = _first_group(m.groups()) - if target: - out.append(target) - for m in ARROW_BOUNDARY.finditer(sentence): - out.append(m.group(1)) - out.extend(BACKTICK.findall(sentence)) - return sorted(set(out)) - -def known_targets(start_dir): - names = set() - # Sibling skills/agents. This is the branch that works in a cache-installed - # plugin and in a deployed .claude/skills/ tree, neither of which has a - # plugins/ directory above it. - parent = os.path.dirname(os.path.abspath(start_dir)) - if os.path.basename(parent) == 'skills' and os.path.isdir(parent): - for entry in os.listdir(parent): - if os.path.isdir(os.path.join(parent, entry)): - names.add(entry) - agents_dir = os.path.join(os.path.dirname(parent), 'agents') - if os.path.isdir(agents_dir): - for entry in os.listdir(agents_dir): - if entry.endswith('.agent.md'): - names.add(entry[:-len('.agent.md')]) - elif entry.endswith('.md'): - names.add(entry[:-len('.md')]) - # Walk up for a monorepo root (plugins/*/.apm/) or a plugin root (.apm/). - # Capped at ten levels so a pathological path can't become a filesystem - # crawl; that covers every real layout by a wide margin. - current = os.path.abspath(start_dir) - for _ in range(10): - # Never glob the filesystem root: a stray /.apm/skills/ (a scaffolding - # test's leftover, say) would otherwise become part of every skill's - # resolution universe on that machine. - if os.path.dirname(current) == current: - break - for pattern in ('plugins/*/.apm/skills/*/', '.apm/skills/*/'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path.rstrip('/'))) - for pattern in ('plugins/*/.apm/agents/*.agent.md', '.apm/agents/*.agent.md'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path)[:-len('.agent.md')]) - current = os.path.dirname(current) - return names - +# The resolution universe comes from the SKILL's own location: the authoring +# root above it (every sibling plugin in the monorepo), its own apm package, and +# the packages that package declares in apm.yml dependencies.apm. It is never +# derived from this script's own path, and — when an authoring root exists — it +# never reads a deployed .claude/ tree, so a fresh clone and a machine that has +# run `apm install` return the same verdict. See the shared resolver's header. if desc: routing_targets = boundary_targets(desc) known = known_targets(skill_dir) if routing_targets else set() - # An empty universe means no authoring source was found anywhere above this - # skill — reporting every target as dangling there would be noise, not a - # finding, so the check declines to run rather than guessing. - if routing_targets and known: - unresolved = [t for t in routing_targets if t not in known] + if routing_targets and not known: + info(f"boundary-target resolution DID NOT RUN — no skill universe could be " + f"determined for this path (no authoring root above it, no apm package " + f"root, no declared apm dependencies, no deployed .claude/ or .agents/ " + f"tree). Unchecked target(s): {', '.join(routing_targets)}") + elif routing_targets: + # blocking vs reported: a target only earns a FAIL when it is written in + # route notation or its own sentence corroborates it by naming another + # target that resolves. See the shared resolver's CORROBORATION note. + unresolved, soft = unresolved_targets(desc, known) for target in unresolved: - fail(f"description routes to '{target}', which resolves to no skill under " - f"plugins/*/.apm/skills/ and no agent under plugins/*/.apm/agents/ — " - f"a boundary clause naming a non-existent target sends the router nowhere") + fail(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — a boundary clause naming a non-existent " + f"target sends the router nowhere") + for target in soft: + suggest(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing " + f"else in that sentence resolves, so it is equally likely to be a tool, a " + f"file format or an English compound. If it IS a route, write it as " + f"`/{target}` or `-> {target}` and it will be checked properly") if not unresolved: - ok(f"all {len(routing_targets)} boundary target(s) resolve: " - f"{', '.join(routing_targets)}") + # Counts the targets that ACTUALLY resolve, not every target found: + # a confirm-only target (one used attributively — see the resolver's + # ATTRIBUTIVE USE note) is exempt from the failure above, so + # reporting it as resolved would be a false claim. + resolved = [t for t in routing_targets if normalize_target(t) in known] + ok(f"{len(resolved)} of {len(routing_targets)} boundary target(s) resolve: " + f"{', '.join(resolved) if resolved else '(none)'}") # Body unfilled placeholders fill_matches = PLACEHOLDER_RE.findall(body) @@ -454,13 +1083,19 @@ if os.path.isdir(scripts_dir): if os.path.isfile(os.path.join(scripts_dir, f)) and not f.endswith('.md')] for fname in scripts: fpath = os.path.join(scripts_dir, fname) - with open(fpath) as f: - sc = f.read() - interactive = interactive_reads(sc) + try: + sc = read_text(fpath) + except EncodingError as exc: + # The executable-bit check below still runs — one unreadable byte + # must not silently drop a second, independent check. + sc = None + fail(f"scripts/{fname}: {exc} — it could not be scanned for " + f"interactive prompts") + interactive = interactive_reads(sc) if sc is not None else [] if interactive: fail(f"scripts/{fname}: may use interactive input " f"(read/input from a terminal detected): {interactive[0]}") - else: + elif sc is not None: ok(f"scripts/{fname}: no interactive prompts detected") # Executable bit if os.access(fpath, os.X_OK): diff --git a/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats b/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats index 568c3e3..f63d799 100755 --- a/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats +++ b/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats @@ -8,7 +8,14 @@ setup() { SCRIPT="$(cd "$BATS_TEST_DIRNAME/../scripts" && pwd)/validate.sh" TMPDIR="$(mktemp -d)" - # Helper: create a minimal valid skill directory + # Helper: create a minimal valid skill directory. + # + # The description carries a boundary clause deliberately. ADR-0020's + # missing-boundary-clause SUGGESTION fires on any description without one, so + # a fixture that omits it is never "otherwise clean" — every test asserting + # SUGGESTION-freedom would be asserting the boundary check's absence instead + # of the thing it names. "anything else" is not hyphenated, so the clause adds + # a boundary marker without adding a routing target to resolve. make_valid_skill() { local dir="$1" local name @@ -17,7 +24,7 @@ setup() { cat > "$dir/SKILL.md" < characters that carries a boundary + # clause and names no routing target. The tests below measure the description + # LENGTH, so the clause has to be paid for out of the same budget rather than + # appended to it — hence the padding arithmetic instead of a fixed suffix. + desc_of_length() { + python3 - "$1" <<'PY' +import sys +n = int(sys.argv[1]) +prefix = 'Use when doing the thing. Do not use for anything else. ' +assert n >= len(prefix), 'requested description shorter than the boundary clause' +print(prefix + 'x' * (n - len(prefix))) +PY + } + # Helper: create a skill directory with an exact description length and an # exact body word count. is used verbatim; "word" # tokens follow the frontmatter. Used by the ADR-0020 boundary tests. @@ -300,7 +321,7 @@ EOF @test "ADR-0020: description of exactly 250 chars raises no suggestion" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "$(python3 -c "print('x' * 250)")" 10 + make_sized_skill "$skill" "$(desc_of_length 250)" 10 run bash "$SCRIPT" "$skill" assert_success refute_output --partial "SUGGESTION" @@ -308,7 +329,7 @@ EOF @test "ADR-0020: description of 251 chars raises a SUGGESTION and still exits 0" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "$(python3 -c "print('x' * 251)")" 10 + make_sized_skill "$skill" "$(desc_of_length 251)" 10 run bash "$SCRIPT" "$skill" assert_success assert_output --partial "SUGGESTION" @@ -318,7 +339,7 @@ EOF @test "ADR-0020: description of exactly 400 chars is a SUGGESTION, not a FAIL" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "$(python3 -c "print('x' * 400)")" 10 + make_sized_skill "$skill" "$(desc_of_length 400)" 10 run bash "$SCRIPT" "$skill" assert_success assert_output --partial "SUGGESTION" @@ -326,7 +347,7 @@ EOF @test "ADR-0020: description of 401 chars FAILs and exits non-zero" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "$(python3 -c "print('x' * 401)")" 10 + make_sized_skill "$skill" "$(desc_of_length 401)" 10 run bash "$SCRIPT" "$skill" assert_failure assert_output --partial "description is 401 chars" @@ -364,7 +385,7 @@ EOF @test "ADR-0020: body of exactly 600 words raises no suggestion" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "A short valid description." 600 + make_sized_skill "$skill" "A short valid description. Do not use for anything else." 600 run bash "$SCRIPT" "$skill" assert_success refute_output --partial "SUGGESTION" @@ -372,7 +393,7 @@ EOF @test "ADR-0020: body of 601 words raises a SUGGESTION and still exits 0" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "A short valid description." 601 + make_sized_skill "$skill" "A short valid description. Do not use for anything else." 601 run bash "$SCRIPT" "$skill" assert_success assert_output --partial "body is 601 words" @@ -381,7 +402,7 @@ EOF @test "ADR-0020: body of exactly 900 words is a SUGGESTION, not a FAIL" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "A short valid description." 900 + make_sized_skill "$skill" "A short valid description. Do not use for anything else." 900 run bash "$SCRIPT" "$skill" assert_success assert_output --partial "body is 900 words" @@ -389,7 +410,7 @@ EOF @test "ADR-0020: body of 901 words FAILs and exits non-zero" { local skill="$TMPDIR/my-skill" - make_sized_skill "$skill" "A short valid description." 901 + make_sized_skill "$skill" "A short valid description. Do not use for anything else." 901 run bash "$SCRIPT" "$skill" assert_failure assert_output --partial "body is 901 words" @@ -425,15 +446,33 @@ EOF assert_output --partial "boundary target(s) resolve" } -@test "ADR-0020: a boundary target naming a non-existent skill FAILs" { +@test "ADR-0020: a boundary target naming a non-existent skill FAILs when its sentence names one that resolves" { local skill skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" - make_sized_skill "$skill" "Use when doing the thing. Do not use for the other thing — use fixture-missing-skill instead." 10 + # `fixture-sibling-skill` is the corroborator: a prose-form target only earns + # a FAIL when its own sentence proves it is a routing sentence. See the + # shared resolver's CORROBORATION note, and the uncorroborated case below. + make_sized_skill "$skill" "Use when doing the thing. Do not use for the other thing — use fixture-sibling-skill or fixture-missing-skill instead." 10 run bash "$SCRIPT" "$skill" assert_failure assert_output --partial "routes to 'fixture-missing-skill'" } +@test "ADR-0020: a LONE boundary target naming a non-existent skill is a SUGGESTION, not a FAIL" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + # Same grammar as the case above and as "run \`pre-commit\` instead" — a + # route verb, a hyphenated name, terminal position. Nothing local separates a + # broken route from a tool name, so the target is named on every run but does + # not block: this gate ships with no baseline and no suppression mechanism. + make_sized_skill "$skill" "Use when doing the thing. Do not use for the other thing — use fixture-missing-skill instead." 10 + run bash "$SCRIPT" "$skill" + assert_success + assert_output --partial "SUGGESTION" + assert_output --partial "routes to 'fixture-missing-skill'" + refute_output --partial "FAIL description routes to" +} + @test "ADR-0020: a boundary target naming an AGENT file resolves (agents are valid routing targets)" { local skill skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" @@ -452,15 +491,27 @@ EOF assert_output --partial "routes to 'fixture-missing-improve'" } -@test "ADR-0020: a backticked name that does not resolve FAILs" { +@test "ADR-0020: a backticked name that does not resolve FAILs when its sentence names one that resolves" { local skill skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" - make_sized_skill "$skill" "Use when doing the thing. Composes \`fixture-missing-helper\` for the shared part." 10 + make_sized_skill "$skill" "Use when doing the thing. Composes \`fixture-sibling-skill\` and \`fixture-missing-helper\` for the shared part." 10 run bash "$SCRIPT" "$skill" assert_failure assert_output --partial "routes to 'fixture-missing-helper'" } +@test "ADR-0020: a /slash-command target is route NOTATION and FAILs on its own, uncorroborated" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + # The escape hatch from the SUGGESTION tier: `/name` and `-> name` are never + # how prose cites a tool, so they are exempt from corroboration. An author + # who wants a route checked unconditionally writes one of those two forms. + make_sized_skill "$skill" "Use when doing the thing. Do not use for the other thing — use /fixture-missing-notation instead." 10 + run bash "$SCRIPT" "$skill" + assert_failure + assert_output --partial "routes to 'fixture-missing-notation'" +} + @test "ADR-0020: a bare hyphenated word outside a boundary sentence is not read as a routing target" { local skill skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" @@ -502,9 +553,18 @@ EOF } @test "ADR-0020: the boundary check declines rather than false-FAILs when no authoring source is found" { + # Deliberately NOT built with make_fixture_tree: this skill sits in a bare + # temp directory with no plugins/*/.apm/ above it and no .git, so the resolver + # legitimately has no universe. That is a real path (a skill being drafted + # outside any repo), and the required behaviour is to DECLINE OUT LOUD rather + # than either false-FAIL or pass in silence — silence is what let a whole gate + # family go missing unnoticed. So the INFO text and the named unchecked target + # are both asserted, not just the absence of a failure. local skill="$TMPDIR/orphan/my-skill" make_sized_skill "$skill" "Use when doing the thing. Do not use for the other thing — use some-other-skill instead." 10 run bash "$SCRIPT" "$skill" assert_success refute_output --partial "routes to" + assert_output --partial "boundary-target resolution DID NOT RUN" + assert_output --partial "Unchecked target(s): some-other-skill" } diff --git a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh index 454e253..5738db4 100755 --- a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh @@ -36,12 +36,38 @@ if [[ $# -lt 1 ]]; then exit 1 fi +# PyYAML is a HARD dependency, not a nice-to-have. The description VALUE has to +# be measured after YAML folding is resolved, and the hand-rolled reader that +# used to stand in for PyYAML disagreed with it across the 400-character FAIL +# boundary — same description, two verdicts, depending on which reader ran. +# Refusing to start is the only honest option; the repo's jq / apm / vale +# dependencies are declared the same way. +# Check the interpreter separately from the library: `python3 -c` fails the same +# way whether python3 is missing or PyYAML is, and reporting the wrong missing +# dependency sends the reader to install the wrong thing. +if ! command -v python3 > /dev/null 2>&1; then + echo "Error: python3 is required but was not found on PATH." >&2 + echo " Why: skipping the ADR-0020 description and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: install python3 (pre-commit itself is a Python application, so it is almost certainly already present)." >&2 + exit 1 +fi + +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "Error: PyYAML is required but is not importable by python3." >&2 + echo " Why: skipping the ADR-0020 description and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" python3 -u - "$1" "$SCRIPT_DIR" <<'PYTHON' import sys import os import re +import glob + +import yaml agent_file = os.path.abspath(sys.argv[1]) script_dir = sys.argv[2] @@ -65,8 +91,18 @@ if not os.path.isfile(inv_path): print(f"Error: field-inventory.md not found at {inv_path}", file=sys.stderr) sys.exit(2) -with open(inv_path) as f: - inv_content = f.read() +# Encoding is pinned to UTF-8 rather than inherited from the locale: under +# LC_ALL=C the inherited default is ASCII, and this file legitimately carries +# non-ASCII prose. read_text() in the shared resolver block below does the same +# thing for every other file; this one is read before that block is defined. +try: + with open(inv_path, encoding='utf-8') as f: + inv_content = f.read() +except UnicodeDecodeError as exc: + print(f"Error: field-inventory.md at {inv_path} is not valid UTF-8 " + f"({exc.reason} at byte {exc.start}) — re-save it as UTF-8.", + file=sys.stderr) + sys.exit(2) def parse_section_tokens(content, section_name): lines = content.splitlines() @@ -112,23 +148,716 @@ failed = False suggestions = [] def fail(msg): + # stderr, matching scripts/skill-size-check.sh's ERROR routing. All three + # scripts in the ADR-0020 family now agree: findings that fail the run go to + # stderr, everything advisory (SUGGESTION / INFO) goes to stdout. Both repo + # callers (check-apm-agents-valid.sh, check-scope-walkup-sync.sh) capture + # `2>&1`, so nothing a human reads moves. global failed failed = True - print(f"FAIL {msg}") + print(f"FAIL {msg}", file=sys.stderr) def suggest(msg): suggestions.append(msg) +def info(msg): + # A check that DECLINED to run says so out loud, rather than passing + # silently. Silence is what let a whole gate family go missing unnoticed. + print(f"INFO {msg}") + PLACEHOLDER_RE = re.compile(r'(?/plugins/*/ — sibling plugins resolve, +# which is what a monorepo means, +# 2. the target's own apm package, +# 3. the packages that package DECLARES in apm.yml dependencies.apm. +# Deployed .claude/ and .agents/ trees are deliberately NOT consulted in that +# case. They are `apm install` output, gitignored, and present only on a machine +# that has run it: four cross-plugin targets in this repo (gitea-branches -> +# git-branches, gitea-branches -> git-history, gitea-issues -> git-branches, +# gitea-workflow -> git-workflow) resolved through .claude/skills/ alone, so the +# same commit measured 2 dangling targets on a developer machine and 6 on a +# fresh clone. A gate shipping hot with no baseline cannot give two answers. +# +# Deployed trees are used only when NO authoring root exists — the consumer +# case, where the file being checked lives in or beside a deployed tree and +# there is no monorepo to read. + + +def _is_fs_root(path): + return os.path.dirname(path) == path + + +def _collect_package(pkg_dir, names): + """Add every skill/agent name a package directory exposes, any layout.""" + for sub in ('.apm/skills/*/', 'skills/*/'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + names.add(os.path.basename(path.rstrip('/')).lower()) + for sub in ('.apm/agents/*.md', 'agents/*.md'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + base = os.path.basename(path) + if base.endswith('.agent.md'): + base = base[:-len('.agent.md')] + else: + base = base[:-len('.md')] + names.add(base.lower()) + + +def _apm_package_root(start_dir): + """Nearest ancestor that is an apm package root (apm.yml or .apm/). + + The filesystem root is never a candidate: a stray /.apm/skills/ — a + scaffolding test's leftover, say, and one really does exist on at least one + machine here — would otherwise become the package root of every path on it. + Capped at ten levels so a pathological path can't become a filesystem + crawl; that covers every real layout by a wide margin. + """ + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + return None + if (os.path.isfile(os.path.join(current, 'apm.yml')) + or os.path.isdir(os.path.join(current, '.apm'))): + return current + current = os.path.dirname(current) + return None + + +def _authoring_root(start_dir): + """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + + Two passes, not one interleaved walk: a nested .git (a submodule, a + worktree of a sub-package) must not win over a real plugins/ root further + up. Both passes stop before the filesystem root for the same reason + _apm_package_root does. + """ + for probe in ( + lambda d: bool(glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))): + current = os.path.abspath(start_dir) + for _ in range(12): + if _is_fs_root(current): + break + if probe(current): + return current + current = os.path.dirname(current) + return None + + +def _collect_authoring_root(root, names): + """Every plugin in the monorepo contributes its names.""" + for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + if os.path.isdir(pkg): + _collect_package(pkg, names) + + +def _declared_dependency_dirs(pkg_dir): + """Directories of the apm packages pkg_dir's manifest DECLARES. + + Reads dependencies.apm and resolves each entry to a directory on disk: + a monorepo-relative `path:` (against the package root and the nearest + ancestor manifest, which is the monorepo root) or an installed + apm_modules//. Entries that resolve to nothing are skipped — an + undeployed dependency contributes no names rather than an error. + """ + manifest = os.path.join(pkg_dir, 'apm.yml') + if not os.path.isfile(manifest): + return [] + try: + data = yaml.safe_load(read_text(manifest)) or {} + except Exception: + return [] + if not isinstance(data, dict): + return [] + deps = data.get('dependencies') + deps = deps.get('apm') if isinstance(deps, dict) else None + if not isinstance(deps, list): + return [] + + roots = [pkg_dir] + ancestor = os.path.dirname(os.path.abspath(pkg_dir)) + for _ in range(10): + if _is_fs_root(ancestor): + break + if os.path.isfile(os.path.join(ancestor, 'apm.yml')): + roots.append(ancestor) + break + ancestor = os.path.dirname(ancestor) + + found = [] + for entry in deps: + candidates = [] + if isinstance(entry, dict): + rel = entry.get('path') + name = entry.get('name') + if not name and rel: + name = os.path.basename(str(rel).rstrip('/')) + if rel: + candidates.extend(os.path.join(r, str(rel)) for r in roots) + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', str(name))) + elif isinstance(entry, str): + name = re.split(r'[#@]', entry)[0].strip().rstrip('/').split('/')[-1] + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', name)) + candidates.extend(os.path.join(r, 'plugins', name) for r in roots) + for candidate in candidates: + if os.path.isdir(candidate): + found.append(candidate) + return found + + +def _deployed_roots(start_dir): + """.claude/ and .agents/ trees above start_dir — what a host really sees. + + Consulted ONLY when no authoring root exists; see the section header. The + filesystem root is skipped for the same reason _apm_package_root skips it: + a stray /.claude/skills/ must not join every path's universe. + """ + found = [] + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + break + for name in ('.claude', '.agents'): + base = os.path.join(current, name) + if os.path.isdir(base): + found.append(base) + current = os.path.dirname(current) + return found + + +def known_targets(start_dir): + """Every skill/agent name a boundary clause in start_dir may name.""" + names = set() + start = os.path.abspath(start_dir) + + # Siblings: a cache-installed plugin and a deployed .claude/skills/ tree + # both put peers one level up, with no plugins/ directory above them. The + # grandparent is guarded against the filesystem root exactly like the two + # walk-up loops above — for a start dir of /skills/ the grandparent is + # `/`, and collecting there picks up this machine's stray /.apm/skills/. + parent = os.path.dirname(start) + grandparent = os.path.dirname(parent) + if (os.path.basename(parent) in ('skills', 'agents') + and os.path.isdir(parent) and not _is_fs_root(grandparent)): + _collect_package(grandparent, names) + + package = _apm_package_root(start) + if package: + _collect_package(package, names) + for dep_dir in _declared_dependency_dirs(package): + _collect_package(dep_dir, names) + + root = _authoring_root(start) + if root: + _collect_authoring_root(root, names) + else: + for base in _deployed_roots(start): + _collect_package(base, names) + return names + + +# --- Extraction ----------------------------------------------------------- +# False positives are the design constraint here, not recall. The rules: +# * A BARE target must be hyphenated AND sit in a boundary sentence (one +# carrying "do not"/"instead"/"rather than"/"not for"). Without the second +# condition, pc-run's "run pre-commit hooks" reads as a route to a +# non-existent `pre-commit` skill. +# * A BARE arrow target counts only in ADR-0020's compressed boundary form, +# `Not -> `. Without that, diagnose's process chain +# "fix -> regression-test" reads as a route to `regression-test`. +# * A backticked hyphenated token counts only inside a boundary sentence. +# Unconditionally, `pre-push` or `commit-msg` in a TRIGGER clause is a hard +# FAIL with no escape hatch. Gating it costs nothing (measured over this +# corpus: 54 targets before and after); DELETING it costs 7 real targets +# across three gitea skills, so it is gated, not removed. +# * SINGLE-WORD targets are deliberately NOT matchable bare — `research`, +# `triage`, `forge`, `prototype` and `tdd` are all real skill names and all +# ordinary English, so a bare-word rule would flag most of the corpus. A +# single-word target must be written `` `forge` `` or /forge to be seen. +# That is a known recall limitation, accepted over the false positives. +# Tool names (Read/Write/Edit) are excluded by the lowercase-only pattern; MCP +# tool names (issue_write) by its rejection of underscores; file names by its +# rejection of dots and slashes. +# +# ATTRIBUTIVE USE. The boundary-sentence gate above does NOT solve the +# `pre-commit` false positive, and the comment that claimed it did was wrong: +# "instead", "rather than", "do not" and "not for" are exactly the words a +# boundary clause uses, so the gate is open precisely where the risk is. All of +# these were hard dangling FAILs with no suppression: +# Use pre-commit hooks instead of ad-hoc scripts. +# Invoke the pull-request template instead of writing one by hand. +# Use conventional-commits formatting rather than free-form messages. +# Composes label-resolution logic instead of duplicating it. +# Do not use for X — run the `pre-push` hooks instead. +# What separates every one of them from a real route is grammar, not marking: +# the hyphenated token is a compound MODIFIER of the noun that follows it +# ("pre-commit hooks", "pull-request template"), where a route target is +# terminal — followed by punctuation, a conjunction, or a boundary word. So a +# target whose next token is an ordinary lowercase noun is CONFIRM-ONLY: it +# still resolves and still counts as a route when the name exists, but it can +# never raise a dangling error on its own. +# +# This is deliberately NOT the simpler "only marked targets may dangle" rule, +# which would have been wrong here: BOTH live true positives in this corpus are +# BARE — research's "(use neuledge-context)" and gitea-issues' "Composes +# gitea-labels-\n milestones", where the `>` fold yields "gitea-labels- +# milestones" and the trailing hyphen is what keeps it terminal. Marking is a +# poor proxy, so the follower token is the signal, and it is applied to +# backticked targets too. +# +# TERMINAL IS NOT ENOUGH — IN-SENTENCE CORROBORATION. The follower test clears +# `pre-push` in the example above only because that example happens to be +# followed by the noun "hooks". Move the same token into terminal position and +# it was a hard FAIL again, with no suppression mechanism anywhere in this gate: +# Do not use for running hooks — run `pre-commit` instead. +# Do not use for the commit message — see `commit-msg`. +# Do not use for type errors — run `type-check` first. +# Instead, use `semantic-release`. +# Do not use for the old flow — use the clean-up instead. +# Do not run end-to-end, run unit-tests. +# Every one of those is grammatically identical to a genuinely broken route: +# "route verb + hyphenated name + terminal" is also exactly how prose cites a +# tool, a hook, a file format or an English compound. Nothing local separates +# them, and the skills most exposed are the ones this contract sends authors +# back to rewrite first — pc-run, pc-author, vale-run, vale-config and the apm-* +# family are all ABOUT hyphenated tools. +# +# So the confidence to BLOCK a commit comes from the sentence, not the token: a +# prose-form target may raise a hard error only when its own sentence names at +# least one OTHER target that RESOLVES. A routing sentence proves itself by +# routing somewhere real; a lone unresolvable name proves nothing. That is not a +# rule fitted to the fixtures — it is the shape of both live true positives, +# which sit beside `write-docs` and `gitea-labels-milestones` respectively, and +# it changes this corpus's verdict by exactly nothing. +# +# An uncorroborated unresolvable target is NOT discarded: every caller reports +# it at its SUGGESTION tier, naming the target. The finding stays visible on +# every run; only the power to block a commit is withdrawn, which is the part +# that had no escape hatch. +# +# EXPLICIT ROUTE NOTATION is exempt from corroboration and always blocks: +# ADR-0020's compressed arrow (`Not -> `) and Claude Code's +# invocation form (`/`). Neither is ever how English cites a tool — nobody +# writes `-> pre-commit` or `/pre-commit` to mean the hook — so there is no +# ambiguity to resolve, and an author who wants a route checked unconditionally +# has two ways to say so. +# +# NAMESPACE: `plugin:skill` is live in this repo (native user-scope installs +# still resolve `gitea:gitea-prs`), so the patterns admit an optional +# `:` prefix and normalize_target() strips it before resolution. +NS = r"(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?" +NAME_ANY = NS + r"[a-z0-9]+(?:-[a-z0-9]+)*" +NAME_HYPH = NS + r"[a-z0-9]+(?:-[a-z0-9]+)+" +ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" + r"|that'?s|compose|composes|call|calls" + r"|routes?\s+to|delegates?\s+to|prefers?|switch(?:es)?\s+to" + r"|hands?\s+off\s+to)") +MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +# A boundary clause takes two shapes and BOTH count: the prose markers, and +# ADR-0020's compressed arrow form `Not -> `. +BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +SENTENCE_SPLIT = re.compile(u'(?<=[.!?])\\s+(?=[A-Z"“(])') + +# The token that may follow a route target without turning it into a compound +# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a +# head noun that names the artifact itself ("the git-workflow skill"). Anything +# else — `hooks`, `template`, `formatting`, `logic` — is attributive prose. +FOLLOWER = re.compile(r"[`\s]*([a-z][a-z0-9]*)") +FOLLOWER_OK = frozenset(""" +and or nor but for to when if unless while after before with from in on at by +of as than then instead rather directly first only always never also even +both either neither so because since per via plus alone here there this that +these those it its they them is are was were be been being has have had will +would can could should must may might does do did +skill skills agent agents plugin plugins command commands +""".split()) + + +def normalize_target(target): + """Comparison key: namespace stripped, lowercased. + + Extraction is case-insensitive (re.I) but the universe is built from + lowercase directory names, so `Git-Commits` at the start of a sentence + resolved to nothing until this normalization existed. + """ + return target.split(':')[-1].lower() + + +def has_boundary_clause(description): + return bool(BOUNDARY_MARKER.search(description) + or BOUNDARY_ARROW.search(description)) + + +def _first(match): + """(name, start, end) offsets for the first group that matched.""" + for index in range(1, (match.re.groups or 0) + 1): + if match.group(index): + return match.group(index), match.start(index), match.end(index) + return None, None, None + + +def _terminal(text, pos): + """True if the token at pos does not make the preceding name a modifier.""" + follower = FOLLOWER.match(text, pos) + return not follower or follower.group(1) in FOLLOWER_OK + + +def _notation(text, start, arrow): + """True if the name is written in route NOTATION rather than in prose. + + Two forms qualify: `/name` (Claude Code's invocation syntax, detected from + the character before the name) and `-> name` (ADR-0020's compressed boundary + form, passed in by the caller that matched the arrow). A backticked name + does NOT qualify — a code span is how a tool, a file and a skill are all + cited, so it carries no intent the follower test hasn't already read. + """ + return arrow or (start > 0 and text[start - 1] == '/') + + +def _add(out, text, name, start, end, strict=None, arrow=False): + if not name: + return + out.append((name, + _terminal(text, end) if strict is None else strict, + _notation(text, start, arrow))) + + +def _scan(text, route_re, cont_re, out): + for match in route_re.finditer(text): + name, start, end = _first(match) + if not name: + continue + _add(out, text, name, start, end) + # "use git-history or git-branches instead" / "use gitea-issues / + # gitea-prs" — keep consuming conjoined targets after the first. + pos = match.end() + while True: + cont = cont_re.match(text, pos) + if not cont: + break + _add(out, text, *_first(cont)) + pos = cont.end() + + +def _extract_sentence(sentence): + """[(name, may_dangle, notation)] for the routing targets in ONE sentence. + + Kept separate from _extract() because corroboration is scoped to a single + sentence: a target's evidence is what stands beside it, not what the rest of + the description happens to mention. + """ + out = [] + boundary = bool(BOUNDARY_MARKER.search(sentence)) + _scan(sentence, + ROUTE_ANY if boundary else ROUTE_MARKED, + CONT_ANY if boundary else CONT_MARKED, + out) + for match in ARROW_MARKED.finditer(sentence): + # `-> name` and `-> /name` are route notation, not prose: nothing + # reads as a compound modifier after an arrow, so no follower test. + _add(out, sentence, *_first(match), strict=True, arrow=True) + for match in ARROW_BOUNDARY.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1), + strict=True, arrow=True) + if boundary: + for match in BACKTICK.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1)) + return out + + +def _extract(description): + """[(name, may_dangle, notation)] for every routing target.""" + out = [] + for sentence in SENTENCE_SPLIT.split(description): + out.extend(_extract_sentence(sentence)) + return out + + +def boundary_targets(description): + """Every routing target, for reporting and for confirming a route.""" + return sorted({name for name, _, _ in _extract(description)}) + + +def unresolved_targets(description, known): + """Targets resolving to nothing, split into (blocking, reported). + + `blocking` earns a hard error; `reported` is SUGGESTION tier — named on + every run, never fatal. Three conditions gate the promotion, and all of them + are documented at length in the ATTRIBUTIVE USE and CORROBORATION notes + above: + + 1. the target must be terminal, not a compound modifier ("pre-commit + hooks" is prose about a tool, not a route), + 2. it must be written in route notation (`/name`, `-> name`), OR + 3. its own sentence must name another target that DOES resolve. + + Everything else is reported and left alone. `known` is the resolved + universe from known_targets(); passing an empty set is not meaningful — + callers check for that first and decline out loud instead. + """ + blocking, reported = set(), set() + for sentence in SENTENCE_SPLIT.split(description): + found = _extract_sentence(sentence) + resolved = {normalize_target(name) for name, _, _ in found + if normalize_target(name) in known} + for name, may_dangle, notation in found: + key = normalize_target(name) + if key in known or not may_dangle: + continue + if notation or (resolved - {key}): + blocking.add(name) + else: + reported.add(name) + return sorted(blocking), sorted(reported - blocking) + +# --- Frontmatter ---------------------------------------------------------- +# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading +# blank line, trailing whitespace after either `---`, or CRLF line endings all +# defeated the old `^---\n(.*?)\n---`, and the miss was SILENT: every ADR-0020 +# check was skipped and the file reported green (measured: a 550-character +# description with a 1,000-word body exited 0 behind a BOM). A file that cannot +# be measured must never report green, so every caller of these two ERRORs on a +# miss instead of moving on. +FRONTMATTER_RE = re.compile( + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + + +def strip_bom(text): + return text[1:] if text.startswith(u'') else text + + +class FrontmatterError(Exception): + pass + + +def description_value(fm_text): + """The description VALUE, with YAML folding resolved. + + PyYAML is a HARD requirement, preflighted in bash. The hand-rolled fallback + this replaced diverged from a real parser across the FAIL boundary — one + corpus description measured 270 characters parsed and 412 unparsed, and a + quoted `"description"` key or an explicit `description: null` returned empty + from it, silently skipping the description AND routing checks. A gate that + disagrees with itself depending on which reader ran is worse than no gate. + + This is the ONLY reader any of the three scripts may use to decide whether a + description is present. A line regex cannot: `description:` with no value + followed by `model: sonnet` lets `\\s*` cross the newline and captures the + NEXT key, which reads as a non-empty description, skips the "missing or + empty" failure, and then early-returns out of every ADR-0020 gate on the + genuinely empty folded value. That combination exited 0 with zero output on + a BLOCKING pre-push gate. + """ + try: + data = yaml.safe_load(fm_text) + except Exception as exc: + raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + if not isinstance(data, dict): + raise FrontmatterError('frontmatter is not a YAML mapping') + value = data.get('description') + if value is None: + return '' + if not isinstance(value, str): + value = str(value) + return re.sub(r'\s+', ' ', value).strip() + + +# --- Body-shape checks (skills only; agents have no references/ dir) ------- +# Deterministic and countable, so they are enforced here. Whether a given +# gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why +# both gotcha checks are SUGGESTION tier. A missing reference file is not a +# style opinion — it is a broken pointer — so that one is ERROR tier. +# +# Both read a FENCE-MASKED copy of the body. Scanning the raw body made a +# ```-fenced example a hard ERROR — and the skills most likely to carry one are +# skill-author and skill-audit, which DOCUMENT the references/ convention — and +# let a `## Gotchas` heading inside a fenced block stand in for the real +# section. Masking preserves every byte offset (content becomes spaces, +# newlines stay), so a span found in the mask slices the original. +GOTCHA_MAX_ENTRIES = 5 +GOTCHA_MAX_BODY_FRACTION = 0.25 +# The heading has to BE "Gotchas", not merely contain the word: `## Gotcha +# handling` and `## Why gotchas matter` are prose sections, and treating one as +# the Gotchas section measured a span that was never a gotcha list. +GOTCHA_HEADING = re.compile(r'^(#{1,6})[ \t]+(?:[^\n]*?[ \t])?gotchas?[ \t]*:?[ \t]*$', + re.I | re.M) +# Column 0 only. `^[ \t]{0,3}` counted a two-space-indented CHILD bullet as a +# top-level entry, so a five-entry section with sub-bullets reported nine. +GOTCHA_ENTRY = re.compile(r'^(?:[-*+]|\d+[.)])[ \t]+', re.M) +FENCE_OPEN = re.compile(r'^[ \t]{0,3}(`{3,}|~{3,})') +REFERENCE_POINTER = re.compile( + r'(?= len(fence) + and not stripped.strip()[len(marker):].strip()): + fence = None + return ''.join(out) + + +def gotcha_stats(body): + """(entry count, section word count) for the first Gotchas section, or None. + + The section runs to the next heading at the same level or shallower. + Entries are top-level list items; a section written as subheadings instead + of a list counts those. Headings and entries are read from the fence mask; + the word count is taken from the original slice, because fenced lines are + real body words and the fraction is measured against the whole body. + """ + masked = mask_fenced(body) + match = GOTCHA_HEADING.search(masked) + if not match: + return None + level = len(match.group(1)) + rest = masked[match.end():] + nxt = re.search(r'^#{1,%d}[ \t]+' % level, rest, re.M) + end = match.end() + (nxt.start() if nxt else len(rest)) + section = masked[match.end():end] + entries = len(GOTCHA_ENTRY.findall(section)) + if entries == 0 and level < 6: + entries = len(re.findall(r'^#{%d,6}[ \t]+' % (level + 1), section, re.M)) + return entries, len(body[match.end():end].split()) + + +def missing_reference_pointers(body, skill_dir): + """references/.md named in the body but absent from disk.""" + masked = mask_fenced(body) + missing = set() + for match in REFERENCE_POINTER.finditer(masked): + start = masked.rfind('\n', 0, match.start()) + 1 + end = masked.find('\n', match.end()) + if end < 0: + end = len(masked) + if REFERENCE_PAST.search(masked[start:end]): + continue + if REFERENCE_QUALIFIER.search(masked[start:match.start()]): + continue + if not os.path.isfile(os.path.join(skill_dir, 'references', match.group(1))): + missing.add('references/' + match.group(1)) + return sorted(missing) +# ===== END ADR-0020 SHARED BOUNDARY RESOLVER ===== + + def parse_frontmatter(content): - m = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) + m = FRONTMATTER_RE.match(strip_bom(content)) if not m: return None, content - return m.group(1), content[m.end():] + return m.group(1), strip_bom(content)[m.end():] def extract_field(fm, field): - m = re.search(rf'^{re.escape(field)}:\s*(.+)', fm, re.MULTILINE) + """The raw text after `field:` ON ITS OWN LINE, or None. + + The character class is `[^\\S\\r\\n]`, never `\\s`: under re.MULTILINE a + `\\s*` after the colon crosses the newline, so `description:` with no value + followed by `model: sonnet` captured `model: sonnet` as the description. + That made the value look present, skipped the "missing or empty" failure, + and then every ADR-0020 gate early-returned on the genuinely empty folded + value — a valueless description exited 0 with zero output on a BLOCKING + pre-push gate. This function is now used only for fields with no folding + semantics (name, tools); description goes through description_value(), the + shared resolver's YAML reader, which is the only thing that can see through + `>`, `null`, `''` and a quoted `"description"` key alike. + """ + m = re.search(rf'^{re.escape(field)}:[^\S\r\n]*(.+)', fm, re.MULTILINE) return m.group(1).strip() if m else None def get_frontmatter_keys(fm): @@ -139,65 +868,17 @@ def get_frontmatter_keys(fm): keys.add(m.group(1)) return keys -def normalize_scalar(value): - return re.sub(r'\s+', ' ', value).strip() - -def fold_description_fallback(fm_text): - """Resolve `description:` without PyYAML. - - Not a YAML parser — it recognises exactly the shapes agent frontmatter - uses: an inline scalar (optionally quoted, optionally continued on - following indented lines) and a `>`/`|` block scalar with optional - indentation and chomping indicators. - """ - lines = fm_text.splitlines() - for i, line in enumerate(lines): - m = re.match(r'^description:[ \t]*(.*)$', line) - if not m: - continue - head = m.group(1).strip() - block = bool(re.match(r'^[>|][0-9]*[-+]?$|^[>|][-+]?[0-9]*$', head)) - parts = [] if block else [head] - for nxt in lines[i + 1:]: - if not nxt.strip(): - parts.append('') - continue - if not re.match(r'^[ \t]', nxt): - break - parts.append(nxt.strip()) - value = ' '.join(parts) - if not block: - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in '"\'': - value = value[1:-1] - return value - return '' - -def description_value(fm): - """The description VALUE with YAML folding resolved. - - extract_field() reads one raw line, which is the right shape for the - presence and placeholder checks but the wrong one for a length gate: a - `>`-folded description measured off its first raw line is not the value the - host preloads. Parse instead of regexing the raw text. - """ +def agent_description(fm, local_fname): + """The folded description VALUE, or None if the frontmatter is not YAML.""" try: - import yaml - data = yaml.safe_load(fm) - if isinstance(data, dict): - value = data.get('description') - if isinstance(value, str): - return normalize_scalar(value) - if value is not None: - return normalize_scalar(str(value)) - return '' - except Exception: - pass - return normalize_scalar(fold_description_fallback(fm)) + return description_value(fm) + except FrontmatterError as exc: + fail(f"frontmatter is not valid YAML ({exc}) — the ADR-0020 description and " + f"boundary-target gates could not run — {local_fname}") + return None -def check_description_budget(fm, local_fname): +def check_description_budget(value, local_fname): """ADR-0020 description gates — identical for every scope.""" - value = description_value(fm) if not value: return dlen = len(value) @@ -213,6 +894,55 @@ def check_description_budget(fm, local_fname): f"what moves the corpus average; the FAIL tier only stops outliers " f"— {local_fname}") +def check_boundary(value, fpath, local_fname): + """ADR-0020 boundary clause + resolvable boundary targets. + + agent-author's SKILL.md states that an agent's boundary targets must + resolve, but until this ran no script checked it — the contract was + documented and unenforced. The resolution universe is derived from the + AGENT FILE's own location (the authoring root above it, its own apm + package, and that package's declared apm dependencies), never from this + script's path, and — when an authoring root exists — never from a deployed + .claude/ tree, so a fresh clone and a machine that has run `apm install` + return the same verdict. + """ + if not value: + return + # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether + # this particular agent warrants a boundary clause is judgment. All four + # agents in this corpus currently lack one. + if not has_boundary_clause(value): + suggest(f"description has no boundary clause — add the prose form (\"Do not use " + f"for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " + f"so the router knows where NOT to send this agent — {local_fname}") + targets = boundary_targets(value) + if not targets: + return + known = known_targets(os.path.dirname(os.path.abspath(fpath))) + if not known: + info(f"boundary-target resolution DID NOT RUN — no skill universe could be " + f"determined for this path (no authoring root above it, no apm package " + f"root, no declared apm dependencies, no deployed .claude/ or .agents/ " + f"tree). Unchecked target(s): {', '.join(targets)} — {local_fname}") + return + # blocking vs reported: a target only earns a FAIL when it is written in + # route notation or its own sentence corroborates it by naming another target + # that resolves. See the shared resolver's CORROBORATION note. + blocking, reported = unresolved_targets(value, known) + for target in blocking: + fail(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — a boundary clause naming a non-existent " + f"target sends the router nowhere — {local_fname}") + for target in reported: + suggest(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing " + f"else in that sentence resolves, so it is equally likely to be a tool, a " + f"file format or an English compound. If it IS a route, write it as " + f"`/{target}` or `-> {target}` and it will be checked properly — " + f"{local_fname}") + def extract_tools_list(fm): """Extract tool names from the tools frontmatter field (space or comma separated).""" val = extract_field(fm, 'tools') @@ -237,7 +967,10 @@ APM_TYPE_RE = re.compile(r"^type:\s*(['\"]?)(instructions|skill|hybrid|prompts)\ def find_apm_package_root(apm_yml_path): """Return True if apm_yml_path has a top-level type: line (i.e. is a package manifest, not a type:-less marketplace-only apm.yml).""" - with open(apm_yml_path) as f: + # errors='replace', not a hard failure: this only asks whether a `type:` + # line exists, and a stray undecodable byte elsewhere in someone else's + # apm.yml must not abort scope detection. + with open(apm_yml_path, encoding='utf-8', errors='replace') as f: for line in f: if APM_TYPE_RE.match(line): return True @@ -273,6 +1006,15 @@ def detect_scope(start_dir): conventional_root = os.path.dirname(os.path.dirname(original_start)) current = original_start while True: + # The filesystem root is never a candidate, the same guard the shared + # resolver's walk-up loops carry. Without it a file under a marker-less + # temp directory walked all the way to `/` and returned it as the scope + # root, which then reported `counterpart file not found: + # /.claude/agents/.md` — a path that names someone else's machine, + # not the user's project. When the walk runs out, the agent file's own + # directory (or its conventional root) is the honest answer. + if _is_fs_root(current): + return 'project', conventional_root if conventional_shape else original_start apm_yml = os.path.join(current, 'apm.yml') if os.path.isfile(apm_yml) and find_apm_package_root(apm_yml): return 'plugin', current @@ -315,12 +1057,19 @@ scope, scope_root = detect_scope(agent_dir) # --- Plugin/APM scope: single vendor-neutral file, no counterpart --- def check_apm_agent_file(fpath, allowlist, stem): local_fname = os.path.basename(fpath) - with open(fpath) as f: - content = f.read() + try: + content = read_text(fpath) + except EncodingError as exc: + fail(f"file is {exc}. Nothing could be measured, so this is a hard " + f"failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: - fail(f"no valid YAML frontmatter (---...---) — {local_fname}") + fail(f"no parseable YAML frontmatter block — expected a `---` line, the fields, " + f"then a closing `---` line (a BOM, leading blank lines, trailing spaces " + f"after either marker and CRLF endings are all tolerated). Nothing could be " + f"measured, so this is a hard failure, not a skip — {local_fname}") return # The apm-agent.md template embeds its authoring guidance as HTML @@ -358,13 +1107,21 @@ def check_apm_agent_file(fpath, allowlist, stem): fail(f"name '{name_val}' does not match filename stem '{stem}' — {local_fname}") # description — required, non-empty, no placeholder - desc_val = extract_field(fm, 'description') - if not desc_val: + # Presence is decided on the FOLDED value, never on a line regex. Deciding + # it on extract_field's raw capture is what let `description:` with no value + # pass this gate in total silence: the capture picked up the next key, so + # "missing or empty" never fired, and every ADR-0020 check below then + # early-returned on the empty folded value. Exit 0, zero output, no gate run. + folded = agent_description(fm, local_fname) + if folded is None: + pass # frontmatter is not valid YAML — agent_description already failed + elif not folded: fail(f"description field is missing or empty — {local_fname}") else: - if PLACEHOLDER_RE.search(desc_val): + if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(fm, local_fname) + check_description_budget(folded, local_fname) + check_boundary(folded, fpath, local_fname) # body — required, non-empty, no placeholder; same Copilot truncation risk # applies since this file compiles verbatim into a real Copilot file downstream. @@ -405,12 +1162,19 @@ else: # user def check_file(fpath, file_provider): local_fname = os.path.basename(fpath) - with open(fpath) as f: - content = f.read() + try: + content = read_text(fpath) + except EncodingError as exc: + fail(f"file is {exc}. Nothing could be measured, so this is a hard " + f"failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: - fail(f"no valid YAML frontmatter (---...---) — {local_fname}") + fail(f"no parseable YAML frontmatter block — expected a `---` line, the fields, " + f"then a closing `---` line (a BOM, leading blank lines, trailing spaces " + f"after either marker and CRLF endings are all tolerated). Nothing could be " + f"measured, so this is a hard failure, not a skip — {local_fname}") return # name — required for CC and Copilot CLI; optional for Copilot cloud/IDE agents @@ -432,13 +1196,21 @@ def check_file(fpath, file_provider): fail(f"name '{name_val}' is not kebab-case — {local_fname}") # description - desc_val = extract_field(fm, 'description') - if not desc_val: + # Presence is decided on the FOLDED value, never on a line regex. Deciding + # it on extract_field's raw capture is what let `description:` with no value + # pass this gate in total silence: the capture picked up the next key, so + # "missing or empty" never fired, and every ADR-0020 check below then + # early-returned on the empty folded value. Exit 0, zero output, no gate run. + folded = agent_description(fm, local_fname) + if folded is None: + pass # frontmatter is not valid YAML — agent_description already failed + elif not folded: fail(f"description field is missing or empty — {local_fname}") else: - if PLACEHOLDER_RE.search(desc_val): + if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(fm, local_fname) + check_description_budget(folded, local_fname) + check_boundary(folded, fpath, local_fname) # body if not body.strip(): diff --git a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh index 6995c5f..35c9733 100755 --- a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh @@ -28,12 +28,37 @@ if [[ $# -lt 1 ]]; then exit 1 fi +# PyYAML is a HARD dependency, not a nice-to-have. The description VALUE has to +# be measured after YAML folding is resolved, and the hand-rolled reader that +# used to stand in for PyYAML disagreed with it across the 400-character FAIL +# boundary — same description, two verdicts, depending on which reader ran. +# Refusing to start is the only honest option; the repo's jq / apm / vale +# dependencies are declared the same way. +# Check the interpreter separately from the library: `python3 -c` fails the same +# way whether python3 is missing or PyYAML is, and reporting the wrong missing +# dependency sends the reader to install the wrong thing. +if ! command -v python3 > /dev/null 2>&1; then + echo "Error: python3 is required but was not found on PATH." >&2 + echo " Why: skipping the ADR-0020 description, body and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: install python3 (pre-commit itself is a Python application, so it is almost certainly already present)." >&2 + exit 1 +fi + +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "Error: PyYAML is required but is not importable by python3." >&2 + echo " Why: skipping the ADR-0020 description, body and boundary-target gates would be a vacuous pass." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi + python3 -u - "$1" <<'PYTHON' import sys import os import re import glob +import yaml + skill_dir = os.path.abspath(sys.argv[1]) skill_md = os.path.join(skill_dir, "SKILL.md") @@ -41,9 +66,6 @@ if not os.path.isfile(skill_md): print(f"Error: '{skill_md}' not found.", file=sys.stderr) sys.exit(1) -with open(skill_md) as f: - content = f.read() - failed = False suggestions = [] @@ -51,8 +73,12 @@ def ok(msg): print(f"PASS {msg}") def fail(msg): + # stderr, matching scripts/skill-size-check.sh's ERROR routing. All three + # scripts in the ADR-0020 family now agree: findings that fail the run go to + # stderr, everything advisory (PASS / SUGGESTION / INFO) goes to stdout. + # Both repo callers capture `2>&1`, so nothing a human reads moves. global failed - print(f"FAIL {msg}") + print(f"FAIL {msg}", file=sys.stderr) failed = True def suggest(msg): @@ -62,71 +88,724 @@ def suggest(msg): # rather than another silently-ignored warning (ADR-0013). suggestions.append(msg) +def info(msg): + # A check that DECLINED to run says so out loud, rather than passing + # silently. Silence is what let a whole gate family go missing unnoticed. + print(f"INFO {msg}") + + +# ===== BEGIN ADR-0020 SHARED BOUNDARY RESOLVER ===== +# ONE resolver, embedded VERBATIM in three scripts: +# scripts/skill-size-check.sh +# plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +# plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +# The block between these markers must stay byte-identical in all three. It is +# copied rather than imported because a cache-installed plugin's scripts cannot +# read files outside their own plugin directory, so there is no single file all +# three can share (same constraint that forces the ADR-0020 constants to be +# duplicated). Edit one copy, then paste it over the other two. +# +# Requires: glob, os, re, yaml (imported by the host script; PyYAML is a hard +# dependency, preflighted in bash before the interpreter starts). + +# --- Input ---------------------------------------------------------------- +# Every file this resolver's callers read goes through read_text(), which pins +# UTF-8 explicitly instead of inheriting locale.getpreferredencoding(). Under +# LC_ALL=C that inherited encoding is ASCII, so a perfectly ordinary em dash in +# a SKILL.md aborted the run with a bare UnicodeDecodeError traceback — loud, +# but pointing at the interpreter rather than at the file or the fix. A file +# that genuinely is not UTF-8 still fails; it just says so. + + +class EncodingError(Exception): + pass + + +def read_text(path): + """File contents as text, UTF-8, with a diagnostic instead of a traceback.""" + try: + with open(path, encoding='utf-8') as fh: + return fh.read() + except UnicodeDecodeError as exc: + raise EncodingError( + "not valid UTF-8 (%s at byte %d) — re-save the file as UTF-8; " + "this gate does not guess at other encodings" + % (exc.reason, exc.start)) + + +# --- Universe ------------------------------------------------------------ +# The set of names a boundary clause may resolve against is derived from an +# AUTHORING ROOT found by walking up FROM THE TARGET FILE. It is NEVER derived +# from this script's own location: deriving it from ${BASH_SOURCE} leaked +# holocron's 39-skill universe into every consumer repo that ran this hook +# through pre-commit, so a consumer skill routing to `skill-audit` resolved +# against a plugin it had never installed. +# +# An authoring root is the nearest ancestor holding plugins/*/.apm/skills/ or +# plugins/*/.apm/agents/ (a plugin monorepo), falling back to the nearest +# ancestor holding .git. When one is found the universe is: +# 1. every skill and agent under /plugins/*/ — sibling plugins resolve, +# which is what a monorepo means, +# 2. the target's own apm package, +# 3. the packages that package DECLARES in apm.yml dependencies.apm. +# Deployed .claude/ and .agents/ trees are deliberately NOT consulted in that +# case. They are `apm install` output, gitignored, and present only on a machine +# that has run it: four cross-plugin targets in this repo (gitea-branches -> +# git-branches, gitea-branches -> git-history, gitea-issues -> git-branches, +# gitea-workflow -> git-workflow) resolved through .claude/skills/ alone, so the +# same commit measured 2 dangling targets on a developer machine and 6 on a +# fresh clone. A gate shipping hot with no baseline cannot give two answers. +# +# Deployed trees are used only when NO authoring root exists — the consumer +# case, where the file being checked lives in or beside a deployed tree and +# there is no monorepo to read. + + +def _is_fs_root(path): + return os.path.dirname(path) == path + + +def _collect_package(pkg_dir, names): + """Add every skill/agent name a package directory exposes, any layout.""" + for sub in ('.apm/skills/*/', 'skills/*/'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + names.add(os.path.basename(path.rstrip('/')).lower()) + for sub in ('.apm/agents/*.md', 'agents/*.md'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + base = os.path.basename(path) + if base.endswith('.agent.md'): + base = base[:-len('.agent.md')] + else: + base = base[:-len('.md')] + names.add(base.lower()) + + +def _apm_package_root(start_dir): + """Nearest ancestor that is an apm package root (apm.yml or .apm/). + + The filesystem root is never a candidate: a stray /.apm/skills/ — a + scaffolding test's leftover, say, and one really does exist on at least one + machine here — would otherwise become the package root of every path on it. + Capped at ten levels so a pathological path can't become a filesystem + crawl; that covers every real layout by a wide margin. + """ + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + return None + if (os.path.isfile(os.path.join(current, 'apm.yml')) + or os.path.isdir(os.path.join(current, '.apm'))): + return current + current = os.path.dirname(current) + return None + + +def _authoring_root(start_dir): + """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + + Two passes, not one interleaved walk: a nested .git (a submodule, a + worktree of a sub-package) must not win over a real plugins/ root further + up. Both passes stop before the filesystem root for the same reason + _apm_package_root does. + """ + for probe in ( + lambda d: bool(glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))): + current = os.path.abspath(start_dir) + for _ in range(12): + if _is_fs_root(current): + break + if probe(current): + return current + current = os.path.dirname(current) + return None + + +def _collect_authoring_root(root, names): + """Every plugin in the monorepo contributes its names.""" + for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + if os.path.isdir(pkg): + _collect_package(pkg, names) + + +def _declared_dependency_dirs(pkg_dir): + """Directories of the apm packages pkg_dir's manifest DECLARES. + + Reads dependencies.apm and resolves each entry to a directory on disk: + a monorepo-relative `path:` (against the package root and the nearest + ancestor manifest, which is the monorepo root) or an installed + apm_modules//. Entries that resolve to nothing are skipped — an + undeployed dependency contributes no names rather than an error. + """ + manifest = os.path.join(pkg_dir, 'apm.yml') + if not os.path.isfile(manifest): + return [] + try: + data = yaml.safe_load(read_text(manifest)) or {} + except Exception: + return [] + if not isinstance(data, dict): + return [] + deps = data.get('dependencies') + deps = deps.get('apm') if isinstance(deps, dict) else None + if not isinstance(deps, list): + return [] + + roots = [pkg_dir] + ancestor = os.path.dirname(os.path.abspath(pkg_dir)) + for _ in range(10): + if _is_fs_root(ancestor): + break + if os.path.isfile(os.path.join(ancestor, 'apm.yml')): + roots.append(ancestor) + break + ancestor = os.path.dirname(ancestor) + + found = [] + for entry in deps: + candidates = [] + if isinstance(entry, dict): + rel = entry.get('path') + name = entry.get('name') + if not name and rel: + name = os.path.basename(str(rel).rstrip('/')) + if rel: + candidates.extend(os.path.join(r, str(rel)) for r in roots) + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', str(name))) + elif isinstance(entry, str): + name = re.split(r'[#@]', entry)[0].strip().rstrip('/').split('/')[-1] + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', name)) + candidates.extend(os.path.join(r, 'plugins', name) for r in roots) + for candidate in candidates: + if os.path.isdir(candidate): + found.append(candidate) + return found + + +def _deployed_roots(start_dir): + """.claude/ and .agents/ trees above start_dir — what a host really sees. + + Consulted ONLY when no authoring root exists; see the section header. The + filesystem root is skipped for the same reason _apm_package_root skips it: + a stray /.claude/skills/ must not join every path's universe. + """ + found = [] + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + break + for name in ('.claude', '.agents'): + base = os.path.join(current, name) + if os.path.isdir(base): + found.append(base) + current = os.path.dirname(current) + return found + + +def known_targets(start_dir): + """Every skill/agent name a boundary clause in start_dir may name.""" + names = set() + start = os.path.abspath(start_dir) + + # Siblings: a cache-installed plugin and a deployed .claude/skills/ tree + # both put peers one level up, with no plugins/ directory above them. The + # grandparent is guarded against the filesystem root exactly like the two + # walk-up loops above — for a start dir of /skills/ the grandparent is + # `/`, and collecting there picks up this machine's stray /.apm/skills/. + parent = os.path.dirname(start) + grandparent = os.path.dirname(parent) + if (os.path.basename(parent) in ('skills', 'agents') + and os.path.isdir(parent) and not _is_fs_root(grandparent)): + _collect_package(grandparent, names) + + package = _apm_package_root(start) + if package: + _collect_package(package, names) + for dep_dir in _declared_dependency_dirs(package): + _collect_package(dep_dir, names) + + root = _authoring_root(start) + if root: + _collect_authoring_root(root, names) + else: + for base in _deployed_roots(start): + _collect_package(base, names) + return names + + +# --- Extraction ----------------------------------------------------------- +# False positives are the design constraint here, not recall. The rules: +# * A BARE target must be hyphenated AND sit in a boundary sentence (one +# carrying "do not"/"instead"/"rather than"/"not for"). Without the second +# condition, pc-run's "run pre-commit hooks" reads as a route to a +# non-existent `pre-commit` skill. +# * A BARE arrow target counts only in ADR-0020's compressed boundary form, +# `Not -> `. Without that, diagnose's process chain +# "fix -> regression-test" reads as a route to `regression-test`. +# * A backticked hyphenated token counts only inside a boundary sentence. +# Unconditionally, `pre-push` or `commit-msg` in a TRIGGER clause is a hard +# FAIL with no escape hatch. Gating it costs nothing (measured over this +# corpus: 54 targets before and after); DELETING it costs 7 real targets +# across three gitea skills, so it is gated, not removed. +# * SINGLE-WORD targets are deliberately NOT matchable bare — `research`, +# `triage`, `forge`, `prototype` and `tdd` are all real skill names and all +# ordinary English, so a bare-word rule would flag most of the corpus. A +# single-word target must be written `` `forge` `` or /forge to be seen. +# That is a known recall limitation, accepted over the false positives. +# Tool names (Read/Write/Edit) are excluded by the lowercase-only pattern; MCP +# tool names (issue_write) by its rejection of underscores; file names by its +# rejection of dots and slashes. +# +# ATTRIBUTIVE USE. The boundary-sentence gate above does NOT solve the +# `pre-commit` false positive, and the comment that claimed it did was wrong: +# "instead", "rather than", "do not" and "not for" are exactly the words a +# boundary clause uses, so the gate is open precisely where the risk is. All of +# these were hard dangling FAILs with no suppression: +# Use pre-commit hooks instead of ad-hoc scripts. +# Invoke the pull-request template instead of writing one by hand. +# Use conventional-commits formatting rather than free-form messages. +# Composes label-resolution logic instead of duplicating it. +# Do not use for X — run the `pre-push` hooks instead. +# What separates every one of them from a real route is grammar, not marking: +# the hyphenated token is a compound MODIFIER of the noun that follows it +# ("pre-commit hooks", "pull-request template"), where a route target is +# terminal — followed by punctuation, a conjunction, or a boundary word. So a +# target whose next token is an ordinary lowercase noun is CONFIRM-ONLY: it +# still resolves and still counts as a route when the name exists, but it can +# never raise a dangling error on its own. +# +# This is deliberately NOT the simpler "only marked targets may dangle" rule, +# which would have been wrong here: BOTH live true positives in this corpus are +# BARE — research's "(use neuledge-context)" and gitea-issues' "Composes +# gitea-labels-\n milestones", where the `>` fold yields "gitea-labels- +# milestones" and the trailing hyphen is what keeps it terminal. Marking is a +# poor proxy, so the follower token is the signal, and it is applied to +# backticked targets too. +# +# TERMINAL IS NOT ENOUGH — IN-SENTENCE CORROBORATION. The follower test clears +# `pre-push` in the example above only because that example happens to be +# followed by the noun "hooks". Move the same token into terminal position and +# it was a hard FAIL again, with no suppression mechanism anywhere in this gate: +# Do not use for running hooks — run `pre-commit` instead. +# Do not use for the commit message — see `commit-msg`. +# Do not use for type errors — run `type-check` first. +# Instead, use `semantic-release`. +# Do not use for the old flow — use the clean-up instead. +# Do not run end-to-end, run unit-tests. +# Every one of those is grammatically identical to a genuinely broken route: +# "route verb + hyphenated name + terminal" is also exactly how prose cites a +# tool, a hook, a file format or an English compound. Nothing local separates +# them, and the skills most exposed are the ones this contract sends authors +# back to rewrite first — pc-run, pc-author, vale-run, vale-config and the apm-* +# family are all ABOUT hyphenated tools. +# +# So the confidence to BLOCK a commit comes from the sentence, not the token: a +# prose-form target may raise a hard error only when its own sentence names at +# least one OTHER target that RESOLVES. A routing sentence proves itself by +# routing somewhere real; a lone unresolvable name proves nothing. That is not a +# rule fitted to the fixtures — it is the shape of both live true positives, +# which sit beside `write-docs` and `gitea-labels-milestones` respectively, and +# it changes this corpus's verdict by exactly nothing. +# +# An uncorroborated unresolvable target is NOT discarded: every caller reports +# it at its SUGGESTION tier, naming the target. The finding stays visible on +# every run; only the power to block a commit is withdrawn, which is the part +# that had no escape hatch. +# +# EXPLICIT ROUTE NOTATION is exempt from corroboration and always blocks: +# ADR-0020's compressed arrow (`Not -> `) and Claude Code's +# invocation form (`/`). Neither is ever how English cites a tool — nobody +# writes `-> pre-commit` or `/pre-commit` to mean the hook — so there is no +# ambiguity to resolve, and an author who wants a route checked unconditionally +# has two ways to say so. +# +# NAMESPACE: `plugin:skill` is live in this repo (native user-scope installs +# still resolve `gitea:gitea-prs`), so the patterns admit an optional +# `:` prefix and normalize_target() strips it before resolution. +NS = r"(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?" +NAME_ANY = NS + r"[a-z0-9]+(?:-[a-z0-9]+)*" +NAME_HYPH = NS + r"[a-z0-9]+(?:-[a-z0-9]+)+" +ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" + r"|that'?s|compose|composes|call|calls" + r"|routes?\s+to|delegates?\s+to|prefers?|switch(?:es)?\s+to" + r"|hands?\s+off\s+to)") +MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +# A boundary clause takes two shapes and BOTH count: the prose markers, and +# ADR-0020's compressed arrow form `Not -> `. +BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +SENTENCE_SPLIT = re.compile(u'(?<=[.!?])\\s+(?=[A-Z"“(])') + +# The token that may follow a route target without turning it into a compound +# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a +# head noun that names the artifact itself ("the git-workflow skill"). Anything +# else — `hooks`, `template`, `formatting`, `logic` — is attributive prose. +FOLLOWER = re.compile(r"[`\s]*([a-z][a-z0-9]*)") +FOLLOWER_OK = frozenset(""" +and or nor but for to when if unless while after before with from in on at by +of as than then instead rather directly first only always never also even +both either neither so because since per via plus alone here there this that +these those it its they them is are was were be been being has have had will +would can could should must may might does do did +skill skills agent agents plugin plugins command commands +""".split()) + + +def normalize_target(target): + """Comparison key: namespace stripped, lowercased. + + Extraction is case-insensitive (re.I) but the universe is built from + lowercase directory names, so `Git-Commits` at the start of a sentence + resolved to nothing until this normalization existed. + """ + return target.split(':')[-1].lower() + + +def has_boundary_clause(description): + return bool(BOUNDARY_MARKER.search(description) + or BOUNDARY_ARROW.search(description)) + + +def _first(match): + """(name, start, end) offsets for the first group that matched.""" + for index in range(1, (match.re.groups or 0) + 1): + if match.group(index): + return match.group(index), match.start(index), match.end(index) + return None, None, None + + +def _terminal(text, pos): + """True if the token at pos does not make the preceding name a modifier.""" + follower = FOLLOWER.match(text, pos) + return not follower or follower.group(1) in FOLLOWER_OK + + +def _notation(text, start, arrow): + """True if the name is written in route NOTATION rather than in prose. + + Two forms qualify: `/name` (Claude Code's invocation syntax, detected from + the character before the name) and `-> name` (ADR-0020's compressed boundary + form, passed in by the caller that matched the arrow). A backticked name + does NOT qualify — a code span is how a tool, a file and a skill are all + cited, so it carries no intent the follower test hasn't already read. + """ + return arrow or (start > 0 and text[start - 1] == '/') + + +def _add(out, text, name, start, end, strict=None, arrow=False): + if not name: + return + out.append((name, + _terminal(text, end) if strict is None else strict, + _notation(text, start, arrow))) + + +def _scan(text, route_re, cont_re, out): + for match in route_re.finditer(text): + name, start, end = _first(match) + if not name: + continue + _add(out, text, name, start, end) + # "use git-history or git-branches instead" / "use gitea-issues / + # gitea-prs" — keep consuming conjoined targets after the first. + pos = match.end() + while True: + cont = cont_re.match(text, pos) + if not cont: + break + _add(out, text, *_first(cont)) + pos = cont.end() + + +def _extract_sentence(sentence): + """[(name, may_dangle, notation)] for the routing targets in ONE sentence. + + Kept separate from _extract() because corroboration is scoped to a single + sentence: a target's evidence is what stands beside it, not what the rest of + the description happens to mention. + """ + out = [] + boundary = bool(BOUNDARY_MARKER.search(sentence)) + _scan(sentence, + ROUTE_ANY if boundary else ROUTE_MARKED, + CONT_ANY if boundary else CONT_MARKED, + out) + for match in ARROW_MARKED.finditer(sentence): + # `-> name` and `-> /name` are route notation, not prose: nothing + # reads as a compound modifier after an arrow, so no follower test. + _add(out, sentence, *_first(match), strict=True, arrow=True) + for match in ARROW_BOUNDARY.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1), + strict=True, arrow=True) + if boundary: + for match in BACKTICK.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1)) + return out + + +def _extract(description): + """[(name, may_dangle, notation)] for every routing target.""" + out = [] + for sentence in SENTENCE_SPLIT.split(description): + out.extend(_extract_sentence(sentence)) + return out + + +def boundary_targets(description): + """Every routing target, for reporting and for confirming a route.""" + return sorted({name for name, _, _ in _extract(description)}) + + +def unresolved_targets(description, known): + """Targets resolving to nothing, split into (blocking, reported). + + `blocking` earns a hard error; `reported` is SUGGESTION tier — named on + every run, never fatal. Three conditions gate the promotion, and all of them + are documented at length in the ATTRIBUTIVE USE and CORROBORATION notes + above: + + 1. the target must be terminal, not a compound modifier ("pre-commit + hooks" is prose about a tool, not a route), + 2. it must be written in route notation (`/name`, `-> name`), OR + 3. its own sentence must name another target that DOES resolve. + + Everything else is reported and left alone. `known` is the resolved + universe from known_targets(); passing an empty set is not meaningful — + callers check for that first and decline out loud instead. + """ + blocking, reported = set(), set() + for sentence in SENTENCE_SPLIT.split(description): + found = _extract_sentence(sentence) + resolved = {normalize_target(name) for name, _, _ in found + if normalize_target(name) in known} + for name, may_dangle, notation in found: + key = normalize_target(name) + if key in known or not may_dangle: + continue + if notation or (resolved - {key}): + blocking.add(name) + else: + reported.add(name) + return sorted(blocking), sorted(reported - blocking) + +# --- Frontmatter ---------------------------------------------------------- +# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading +# blank line, trailing whitespace after either `---`, or CRLF line endings all +# defeated the old `^---\n(.*?)\n---`, and the miss was SILENT: every ADR-0020 +# check was skipped and the file reported green (measured: a 550-character +# description with a 1,000-word body exited 0 behind a BOM). A file that cannot +# be measured must never report green, so every caller of these two ERRORs on a +# miss instead of moving on. +FRONTMATTER_RE = re.compile( + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + + +def strip_bom(text): + return text[1:] if text.startswith(u'') else text + + +class FrontmatterError(Exception): + pass + + +def description_value(fm_text): + """The description VALUE, with YAML folding resolved. + + PyYAML is a HARD requirement, preflighted in bash. The hand-rolled fallback + this replaced diverged from a real parser across the FAIL boundary — one + corpus description measured 270 characters parsed and 412 unparsed, and a + quoted `"description"` key or an explicit `description: null` returned empty + from it, silently skipping the description AND routing checks. A gate that + disagrees with itself depending on which reader ran is worse than no gate. + + This is the ONLY reader any of the three scripts may use to decide whether a + description is present. A line regex cannot: `description:` with no value + followed by `model: sonnet` lets `\\s*` cross the newline and captures the + NEXT key, which reads as a non-empty description, skips the "missing or + empty" failure, and then early-returns out of every ADR-0020 gate on the + genuinely empty folded value. That combination exited 0 with zero output on + a BLOCKING pre-push gate. + """ + try: + data = yaml.safe_load(fm_text) + except Exception as exc: + raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + if not isinstance(data, dict): + raise FrontmatterError('frontmatter is not a YAML mapping') + value = data.get('description') + if value is None: + return '' + if not isinstance(value, str): + value = str(value) + return re.sub(r'\s+', ' ', value).strip() + + +# --- Body-shape checks (skills only; agents have no references/ dir) ------- +# Deterministic and countable, so they are enforced here. Whether a given +# gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why +# both gotcha checks are SUGGESTION tier. A missing reference file is not a +# style opinion — it is a broken pointer — so that one is ERROR tier. +# +# Both read a FENCE-MASKED copy of the body. Scanning the raw body made a +# ```-fenced example a hard ERROR — and the skills most likely to carry one are +# skill-author and skill-audit, which DOCUMENT the references/ convention — and +# let a `## Gotchas` heading inside a fenced block stand in for the real +# section. Masking preserves every byte offset (content becomes spaces, +# newlines stay), so a span found in the mask slices the original. +GOTCHA_MAX_ENTRIES = 5 +GOTCHA_MAX_BODY_FRACTION = 0.25 +# The heading has to BE "Gotchas", not merely contain the word: `## Gotcha +# handling` and `## Why gotchas matter` are prose sections, and treating one as +# the Gotchas section measured a span that was never a gotcha list. +GOTCHA_HEADING = re.compile(r'^(#{1,6})[ \t]+(?:[^\n]*?[ \t])?gotchas?[ \t]*:?[ \t]*$', + re.I | re.M) +# Column 0 only. `^[ \t]{0,3}` counted a two-space-indented CHILD bullet as a +# top-level entry, so a five-entry section with sub-bullets reported nine. +GOTCHA_ENTRY = re.compile(r'^(?:[-*+]|\d+[.)])[ \t]+', re.M) +FENCE_OPEN = re.compile(r'^[ \t]{0,3}(`{3,}|~{3,})') +REFERENCE_POINTER = re.compile( + r'(?= len(fence) + and not stripped.strip()[len(marker):].strip()): + fence = None + return ''.join(out) + + +def gotcha_stats(body): + """(entry count, section word count) for the first Gotchas section, or None. + + The section runs to the next heading at the same level or shallower. + Entries are top-level list items; a section written as subheadings instead + of a list counts those. Headings and entries are read from the fence mask; + the word count is taken from the original slice, because fenced lines are + real body words and the fraction is measured against the whole body. + """ + masked = mask_fenced(body) + match = GOTCHA_HEADING.search(masked) + if not match: + return None + level = len(match.group(1)) + rest = masked[match.end():] + nxt = re.search(r'^#{1,%d}[ \t]+' % level, rest, re.M) + end = match.end() + (nxt.start() if nxt else len(rest)) + section = masked[match.end():end] + entries = len(GOTCHA_ENTRY.findall(section)) + if entries == 0 and level < 6: + entries = len(re.findall(r'^#{%d,6}[ \t]+' % (level + 1), section, re.M)) + return entries, len(body[match.end():end].split()) + + +def missing_reference_pointers(body, skill_dir): + """references/.md named in the body but absent from disk.""" + masked = mask_fenced(body) + missing = set() + for match in REFERENCE_POINTER.finditer(masked): + start = masked.rfind('\n', 0, match.start()) + 1 + end = masked.find('\n', match.end()) + if end < 0: + end = len(masked) + if REFERENCE_PAST.search(masked[start:end]): + continue + if REFERENCE_QUALIFIER.search(masked[start:match.start()]): + continue + if not os.path.isfile(os.path.join(skill_dir, 'references', match.group(1))): + missing.add('references/' + match.group(1)) + return sorted(missing) +# ===== END ADR-0020 SHARED BOUNDARY RESOLVER ===== + + +# A leading BOM is stripped before anything is parsed or counted. It changes +# neither count below — it is not a line separator and str.split() does not +# treat it as whitespace — but it did defeat the frontmatter match. +try: + content = strip_bom(read_text(skill_md)) +except EncodingError as exc: + fail(f"SKILL.md is {exc}. Nothing downstream can be measured, so this is a " + f"hard failure, not a skip") + print("One or more checks failed.") + sys.exit(1) + # --- Parse frontmatter --- -fm_match = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) +fm_match = FRONTMATTER_RE.match(content) if not fm_match: - fail("No valid YAML frontmatter block found (expected ---...---)") + fail("No parseable YAML frontmatter block found. Expected a `---` line, the " + "fields, then a closing `---` line (a BOM, leading blank lines, trailing " + "spaces after either marker and CRLF endings are all tolerated). Nothing " + "downstream can be measured, so this is a hard failure, not a skip") + print("One or more checks failed.") sys.exit(1) fm = fm_match.group(1) body_start = fm_match.end() -# Extract name -name_m = re.search(r'^name:\s*(\S+)', fm, re.MULTILINE) +# Extract name. The character class is `[ \t]`, never `\s`: under re.MULTILINE +# a `\s*` after the colon crosses the newline, so a valueless `name:` followed +# by `description: ...` captured the NEXT KEY as the name and reported a +# mismatch instead of an absence. Same class of bug as the `description:` one +# the shared resolver's description_value() docstring records. +name_m = re.search(r'^name:[ \t]*(\S+)', fm, re.MULTILINE) name = name_m.group(1).strip('"\'') if name_m else "" # Extract description — the VALUE, with YAML folding resolved. Most of this # corpus writes descriptions as `>`-folded block scalars, so the raw lines # carry indentation and newlines that are not part of the value: every length -# measurement below is wrong unless the scalar is folded first. PyYAML is used -# when importable (it is a real parser); the fallback recognises exactly the -# shapes this corpus uses — an inline scalar, optionally quoted and optionally -# continued on following indented lines, and a `>`/`|` block scalar with -# optional indentation and chomping indicators. - -def normalize(value): - return re.sub(r'\s+', ' ', value).strip() - -def fold_description_fallback(fm_text): - lines = fm_text.splitlines() - for i, line in enumerate(lines): - m = re.match(r'^description:[ \t]*(.*)$', line) - if not m: - continue - head = m.group(1).strip() - block = bool(re.match(r'^[>|][0-9]*[-+]?$|^[>|][-+]?[0-9]*$', head)) - parts = [] if block else [head] - for nxt in lines[i + 1:]: - if not nxt.strip(): - parts.append('') - continue - if not re.match(r'^[ \t]', nxt): - break - parts.append(nxt.strip()) - value = ' '.join(parts) - if not block: - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in '"\'': - value = value[1:-1] - return value - return '' - -def extract_description(fm_text): - try: - import yaml - data = yaml.safe_load(fm_text) - if isinstance(data, dict): - value = data.get('description') - if isinstance(value, str): - return normalize(value) - if value is not None: - return normalize(str(value)) - return '' - except Exception: - pass - return normalize(fold_description_fallback(fm_text)) - -desc = extract_description(fm) +# measurement below is wrong unless the scalar is folded first. +try: + desc = description_value(fm) +except FrontmatterError as exc: + fail(f"frontmatter is not valid YAML ({exc}). Nothing downstream can be " + f"measured, so this is a hard failure, not a skip") + print("One or more checks failed.") + sys.exit(1) dir_name = os.path.basename(skill_dir) @@ -155,13 +834,13 @@ if name: # name format if name: if re.match(r'^[a-z0-9]+(-[a-z0-9]+)*$', name): - ok(f"name format valid (kebab-case)") + ok("name format valid (kebab-case)") else: fail(f"name '{name}' is invalid — use lowercase letters, numbers, and hyphens only; no leading, trailing, or consecutive hyphens") # description present if desc: - ok(f"description present") + ok("description present") else: fail("description field is missing or empty") @@ -270,135 +949,85 @@ elif body_word_count > BODY_SUGGEST_WORDS: else: ok(f"SKILL.md body word count {body_word_count} (ADR-0020 target: {BODY_SUGGEST_WORDS})") +# --- Reference pointers must exist ----------------------------------------- +# FAIL, not SUGGESTION: a dispatch table naming a references/ file that is not +# on disk is a hard break, and until this check existed nothing in the +# gate/audit/vale stack noticed it — all three exited 0. +missing_refs = missing_reference_pointers(body, skill_dir) +for ref in missing_refs: + fail(f"SKILL.md body points at {ref}, which does not exist on disk — a dispatch " + f"table or \"read X\" trigger naming a missing file sends the agent nowhere") +if not missing_refs: + ok("all referenced references/ files exist") + +# --- Gotchas discipline ----------------------------------------------------- +# SUGGESTION on both counts: the measurement is deterministic, but whether a +# given gotcha earns its place in the body is the auditor's judgment. +gotchas = gotcha_stats(body) +if gotchas is not None: + gotcha_entries, gotcha_words = gotchas + if gotcha_entries > GOTCHA_MAX_ENTRIES: + suggest(f"Gotchas section has {gotcha_entries} entries — over the " + f"{GOTCHA_MAX_ENTRIES}-entry guideline. A list that long is usually a " + f"missing references/ file or a design problem written up as a warning") + if body_word_count and gotcha_words > body_word_count * GOTCHA_MAX_BODY_FRACTION: + suggest(f"Gotchas section is {gotcha_words} of {body_word_count} body words " + f"({round(100.0 * gotcha_words / body_word_count)}%) — over the " + f"{round(100.0 * GOTCHA_MAX_BODY_FRACTION)}% guideline. Move the durable " + f"parts to references/ and keep the section for live traps") + +# --- ADR-0020: boundary clause present ------------------------------------- +# SUGGESTION, not FAIL: detecting the absence is deterministic, but whether +# this particular skill warrants a boundary clause is judgment. Both accepted +# shapes count — the prose markers and the compressed `Not -> `. +if desc: + if has_boundary_clause(desc): + ok("description has a boundary clause") + else: + suggest("description has no boundary clause — add the prose form (\"Do not use " + "for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " + "so the router knows where NOT to send this skill") + # --- ADR-0020: resolvable boundary targets --------------------------------- -# A boundary clause names another skill — or an agent, which is an equally -# valid routing target (git-workflow routes to the git-orchestrate agent). Every -# named target is resolved against the AUTHORING SOURCE, plugins/*/.apm/skills/ -# and plugins/*/.apm/agents/, so the check works offline and before an -# `apm install` has deployed anything into .claude/skills/. -# -# False positives are the design constraint here, not recall. Two rules do the -# work: -# * A BARE hyphenated word is read as a routing target only inside a boundary -# sentence (one carrying "do not"/"instead"/"rather than"/"not for"). -# Without that, pc-run's "run pre-commit hooks" reads as a route to a -# non-existent `pre-commit` skill. -# * A BARE arrow target counts only in ADR-0020's compressed boundary form, -# `Not -> `. Without that, diagnose's process chain -# "fix -> regression-test" reads as a route to `regression-test`. -# Backticked and /slash-command targets are unambiguous and always count. Tool -# names (Read, Write, Edit) are excluded by the lowercase-only name pattern; -# MCP tool names (issue_write, pull_request_write) by its rejection of -# underscores; file names by its rejection of dots and slashes. - -NAME_ANY = r"[a-z0-9]+(?:-[a-z0-9]+)*" -NAME_HYPH = r"[a-z0-9]+(?:-[a-z0-9]+)+" -ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" - r"|that'?s|compose|composes|call|calls)") -MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) -BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -SENTENCE_SPLIT = re.compile('(?<=[.!?])\\s+(?=[A-Z"“(])') - -def _first_group(groups): - for g in groups: - if g: - return g - return None - -def _scan_routes(text, route_re, cont_re, out): - for m in route_re.finditer(text): - target = _first_group(m.groups()) - if not target: - continue - out.append(target) - # Conjoined targets: "use git-history or git-branches instead", - # "use gitea-issues / gitea-prs". - pos = m.end() - while True: - cm = cont_re.match(text, pos) - if not cm: - break - nxt = _first_group(cm.groups()) - if nxt: - out.append(nxt) - pos = cm.end() - -def boundary_targets(description): - out = [] - for sentence in SENTENCE_SPLIT.split(description): - boundary = bool(BOUNDARY_MARKER.search(sentence)) - _scan_routes(sentence, - ROUTE_ANY if boundary else ROUTE_MARKED, - CONT_ANY if boundary else CONT_MARKED, - out) - for m in ARROW_MARKED.finditer(sentence): - target = _first_group(m.groups()) - if target: - out.append(target) - for m in ARROW_BOUNDARY.finditer(sentence): - out.append(m.group(1)) - out.extend(BACKTICK.findall(sentence)) - return sorted(set(out)) - -def known_targets(start_dir): - names = set() - # Sibling skills/agents. This is the branch that works in a cache-installed - # plugin and in a deployed .claude/skills/ tree, neither of which has a - # plugins/ directory above it. - parent = os.path.dirname(os.path.abspath(start_dir)) - if os.path.basename(parent) == 'skills' and os.path.isdir(parent): - for entry in os.listdir(parent): - if os.path.isdir(os.path.join(parent, entry)): - names.add(entry) - agents_dir = os.path.join(os.path.dirname(parent), 'agents') - if os.path.isdir(agents_dir): - for entry in os.listdir(agents_dir): - if entry.endswith('.agent.md'): - names.add(entry[:-len('.agent.md')]) - elif entry.endswith('.md'): - names.add(entry[:-len('.md')]) - # Walk up for a monorepo root (plugins/*/.apm/) or a plugin root (.apm/). - # Capped at ten levels so a pathological path can't become a filesystem - # crawl; that covers every real layout by a wide margin. - current = os.path.abspath(start_dir) - for _ in range(10): - # Never glob the filesystem root: a stray /.apm/skills/ (a scaffolding - # test's leftover, say) would otherwise become part of every skill's - # resolution universe on that machine. - if os.path.dirname(current) == current: - break - for pattern in ('plugins/*/.apm/skills/*/', '.apm/skills/*/'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path.rstrip('/'))) - for pattern in ('plugins/*/.apm/agents/*.agent.md', '.apm/agents/*.agent.md'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path)[:-len('.agent.md')]) - current = os.path.dirname(current) - return names - +# The resolution universe comes from the SKILL's own location: the authoring +# root above it (every sibling plugin in the monorepo), its own apm package, and +# the packages that package declares in apm.yml dependencies.apm. It is never +# derived from this script's own path, and — when an authoring root exists — it +# never reads a deployed .claude/ tree, so a fresh clone and a machine that has +# run `apm install` return the same verdict. See the shared resolver's header. if desc: routing_targets = boundary_targets(desc) known = known_targets(skill_dir) if routing_targets else set() - # An empty universe means no authoring source was found anywhere above this - # skill — reporting every target as dangling there would be noise, not a - # finding, so the check declines to run rather than guessing. - if routing_targets and known: - unresolved = [t for t in routing_targets if t not in known] + if routing_targets and not known: + info(f"boundary-target resolution DID NOT RUN — no skill universe could be " + f"determined for this path (no authoring root above it, no apm package " + f"root, no declared apm dependencies, no deployed .claude/ or .agents/ " + f"tree). Unchecked target(s): {', '.join(routing_targets)}") + elif routing_targets: + # blocking vs reported: a target only earns a FAIL when it is written in + # route notation or its own sentence corroborates it by naming another + # target that resolves. See the shared resolver's CORROBORATION note. + unresolved, soft = unresolved_targets(desc, known) for target in unresolved: - fail(f"description routes to '{target}', which resolves to no skill under " - f"plugins/*/.apm/skills/ and no agent under plugins/*/.apm/agents/ — " - f"a boundary clause naming a non-existent target sends the router nowhere") + fail(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — a boundary clause naming a non-existent " + f"target sends the router nowhere") + for target in soft: + suggest(f"description routes to '{target}', which resolves to no skill or agent " + f"in this monorepo, in this package, or in a package it declares in " + f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing " + f"else in that sentence resolves, so it is equally likely to be a tool, a " + f"file format or an English compound. If it IS a route, write it as " + f"`/{target}` or `-> {target}` and it will be checked properly") if not unresolved: - ok(f"all {len(routing_targets)} boundary target(s) resolve: " - f"{', '.join(routing_targets)}") + # Counts the targets that ACTUALLY resolve, not every target found: + # a confirm-only target (one used attributively — see the resolver's + # ATTRIBUTIVE USE note) is exempt from the failure above, so + # reporting it as resolved would be a false claim. + resolved = [t for t in routing_targets if normalize_target(t) in known] + ok(f"{len(resolved)} of {len(routing_targets)} boundary target(s) resolve: " + f"{', '.join(resolved) if resolved else '(none)'}") # Body unfilled placeholders fill_matches = PLACEHOLDER_RE.findall(body) @@ -454,13 +1083,19 @@ if os.path.isdir(scripts_dir): if os.path.isfile(os.path.join(scripts_dir, f)) and not f.endswith('.md')] for fname in scripts: fpath = os.path.join(scripts_dir, fname) - with open(fpath) as f: - sc = f.read() - interactive = interactive_reads(sc) + try: + sc = read_text(fpath) + except EncodingError as exc: + # The executable-bit check below still runs — one unreadable byte + # must not silently drop a second, independent check. + sc = None + fail(f"scripts/{fname}: {exc} — it could not be scanned for " + f"interactive prompts") + interactive = interactive_reads(sc) if sc is not None else [] if interactive: fail(f"scripts/{fname}: may use interactive input " f"(read/input from a terminal detected): {interactive[0]}") - else: + elif sc is not None: ok(f"scripts/{fname}: no interactive prompts detected") # Executable bit if os.access(fpath, os.X_OK): diff --git a/scripts/skill-size-check.sh b/scripts/skill-size-check.sh index 8311173..906535b 100755 --- a/scripts/skill-size-check.sh +++ b/scripts/skill-size-check.sh @@ -52,10 +52,12 @@ set -euo pipefail # tokenization and does not replace one. Re-measure the corpus before treating # any of these numbers as still current. # -# python3 is required for the ADR-0020 half. That is not a new dependency in -# practice: pre-commit, which is how this script runs, is itself a Python -# application. PyYAML is used when importable and is genuinely optional — the -# fallback reader below recognises the frontmatter shapes this corpus uses. +# python3 AND PyYAML are required for the ADR-0020 half, and both are hard +# dependencies rather than best-effort: python3 because pre-commit (which is how +# this script runs) is itself a Python application, and PyYAML because the +# hand-rolled folding reader that used to cover its absence disagreed with a +# real parser across the FAIL boundary. Two readers that measure the same +# description differently is worse than one reader that refuses to start. # These constants are intentionally duplicated in # skill-audit/scripts/validate.sh (Python) rather than shared from one file: @@ -84,7 +86,24 @@ BODY_MAX_WORDS=900 FAIL=0 for f in "$@"; do - [[ -f "$f" ]] || continue + # NOT a silent skip — see the matching note on the Python side. A broken + # symlink named SKILL.md is storable in git and a directory named SKILL.md + # reaches this hook the same way; both used to make the whole run exit 0 with + # no output at all, which is the one thing this script must never do. + if [[ ! -f "$f" ]]; then + if [[ -d "$f" ]]; then + why="is a directory, not a file" + elif [[ -L "$f" ]]; then + why="is a symlink that does not resolve to a file" + elif [[ -e "$f" ]]; then + why="is not a regular file" + else + why="does not exist" + fi + echo "ERROR: $f $why, so the line and word ceilings could not be measured. A path this hook was handed and could not read does not get to pass in silence." >&2 + FAIL=1 + continue + fi # Single awk pass computes both line count and word count, avoiding a # second read of the file. NR counts the final line even without a @@ -112,26 +131,29 @@ if ! command -v python3 > /dev/null 2>&1; then exit 1 fi -# REPO_ROOT is passed in so the boundary-target resolver has one guaranteed -# place to look for the authoring source (plugins/*/.apm/skills/), independent -# of the cwd pre-commit happens to invoke this hook from. -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "ERROR: PyYAML is required for the ADR-0020 description/body/boundary-target gates but is not importable by python3." >&2 + echo " Why: the description VALUE has to be measured after YAML folding is resolved, and the hand-rolled reader that used to stand in for PyYAML disagreed with it across the 400-character FAIL boundary. Falling back would make the verdict depend on which reader ran." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi if ! python3 -u - \ - "$REPO_ROOT" "$DESC_SUGGEST_CHARS" "$DESC_MAX_CHARS" \ + "$DESC_SUGGEST_CHARS" "$DESC_MAX_CHARS" \ "$BODY_SUGGEST_WORDS" "$BODY_MAX_WORDS" "$MAX_WORDS" "$@" <<'PYTHON' import glob import os import re import sys -repo_root = sys.argv[1] -DESC_SUGGEST_CHARS = int(sys.argv[2]) -DESC_MAX_CHARS = int(sys.argv[3]) -BODY_SUGGEST_WORDS = int(sys.argv[4]) -BODY_MAX_WORDS = int(sys.argv[5]) -MAX_WORDS = int(sys.argv[6]) -files = sys.argv[7:] +import yaml + +DESC_SUGGEST_CHARS = int(sys.argv[1]) +DESC_MAX_CHARS = int(sys.argv[2]) +BODY_SUGGEST_WORDS = int(sys.argv[3]) +BODY_MAX_WORDS = int(sys.argv[4]) +MAX_WORDS = int(sys.argv[5]) +files = sys.argv[6:] failed = False @@ -150,117 +172,350 @@ def suggest(msg): print("SUGGESTION: %s" % msg) -# --- Frontmatter / description / body ------------------------------------ -# The description VALUE must be measured after YAML folding is resolved: this -# corpus writes most descriptions as `>`-folded block scalars, so the raw lines -# carry indentation and newlines that are not part of the value. Parse, don't -# regex the raw text. - -def normalize(value): - return re.sub(r'\s+', ' ', value).strip() +def info(msg): + # A check that DECLINED to run says so out loud. The one thing this script + # must never do is stay quiet about a measurement it did not take. + print("INFO: %s" % msg) -def fold_description_fallback(fm_text): - """Resolve `description:` without PyYAML. +# ===== BEGIN ADR-0020 SHARED BOUNDARY RESOLVER ===== +# ONE resolver, embedded VERBATIM in three scripts: +# scripts/skill-size-check.sh +# plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +# plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +# The block between these markers must stay byte-identical in all three. It is +# copied rather than imported because a cache-installed plugin's scripts cannot +# read files outside their own plugin directory, so there is no single file all +# three can share (same constraint that forces the ADR-0020 constants to be +# duplicated). Edit one copy, then paste it over the other two. +# +# Requires: glob, os, re, yaml (imported by the host script; PyYAML is a hard +# dependency, preflighted in bash before the interpreter starts). - Not a YAML parser — it recognises exactly the shapes this corpus uses: - an inline scalar (optionally quoted, optionally continued on following - indented lines) and a `>`/`|` block scalar with optional indentation and - chomping indicators. - """ - lines = fm_text.splitlines() - for i, line in enumerate(lines): - m = re.match(r'^description:[ \t]*(.*)$', line) - if not m: - continue - head = m.group(1).strip() - block = bool(re.match(r'^[>|][0-9]*[-+]?$|^[>|][-+]?[0-9]*$', head)) - parts = [] if block else [head] - for nxt in lines[i + 1:]: - if not nxt.strip(): - parts.append('') - continue - if not re.match(r'^[ \t]', nxt): - break - parts.append(nxt.strip()) - value = ' '.join(parts) - if not block: - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in '"\'': - value = value[1:-1] - return value - return '' +# --- Input ---------------------------------------------------------------- +# Every file this resolver's callers read goes through read_text(), which pins +# UTF-8 explicitly instead of inheriting locale.getpreferredencoding(). Under +# LC_ALL=C that inherited encoding is ASCII, so a perfectly ordinary em dash in +# a SKILL.md aborted the run with a bare UnicodeDecodeError traceback — loud, +# but pointing at the interpreter rather than at the file or the fix. A file +# that genuinely is not UTF-8 still fails; it just says so. -def extract_description(fm_text): +class EncodingError(Exception): + pass + + +def read_text(path): + """File contents as text, UTF-8, with a diagnostic instead of a traceback.""" try: - import yaml - data = yaml.safe_load(fm_text) - if isinstance(data, dict): - value = data.get('description') - if isinstance(value, str): - return normalize(value) - if value is not None: - return normalize(str(value)) - return '' - except Exception: - pass - return normalize(fold_description_fallback(fm_text)) + with open(path, encoding='utf-8') as fh: + return fh.read() + except UnicodeDecodeError as exc: + raise EncodingError( + "not valid UTF-8 (%s at byte %d) — re-save the file as UTF-8; " + "this gate does not guess at other encodings" + % (exc.reason, exc.start)) -# --- Boundary-target resolution ------------------------------------------ -# A description's boundary clause names another skill (or an agent — agents are -# legitimate routing targets: git-workflow routes to git-orchestrate). Resolve -# every named target against the AUTHORING SOURCE, plugins/*/.apm/skills/ and -# plugins/*/.apm/agents/, so the check works offline and before `apm install` -# has deployed anything into .claude/skills/. +# --- Universe ------------------------------------------------------------ +# The set of names a boundary clause may resolve against is derived from an +# AUTHORING ROOT found by walking up FROM THE TARGET FILE. It is NEVER derived +# from this script's own location: deriving it from ${BASH_SOURCE} leaked +# holocron's 39-skill universe into every consumer repo that ran this hook +# through pre-commit, so a consumer skill routing to `skill-audit` resolved +# against a plugin it had never installed. +# +# An authoring root is the nearest ancestor holding plugins/*/.apm/skills/ or +# plugins/*/.apm/agents/ (a plugin monorepo), falling back to the nearest +# ancestor holding .git. When one is found the universe is: +# 1. every skill and agent under /plugins/*/ — sibling plugins resolve, +# which is what a monorepo means, +# 2. the target's own apm package, +# 3. the packages that package DECLARES in apm.yml dependencies.apm. +# Deployed .claude/ and .agents/ trees are deliberately NOT consulted in that +# case. They are `apm install` output, gitignored, and present only on a machine +# that has run it: four cross-plugin targets in this repo (gitea-branches -> +# git-branches, gitea-branches -> git-history, gitea-issues -> git-branches, +# gitea-workflow -> git-workflow) resolved through .claude/skills/ alone, so the +# same commit measured 2 dangling targets on a developer machine and 6 on a +# fresh clone. A gate shipping hot with no baseline cannot give two answers. +# +# Deployed trees are used only when NO authoring root exists — the consumer +# case, where the file being checked lives in or beside a deployed tree and +# there is no monorepo to read. -def known_targets(start_dir): - names = set() - # Sibling skills/agents: covers a cache-installed plugin and a deployed - # .claude/skills/ tree, neither of which has a plugins/ directory above it. - parent = os.path.dirname(os.path.abspath(start_dir)) - if os.path.basename(parent) == 'skills' and os.path.isdir(parent): - for entry in os.listdir(parent): - if os.path.isdir(os.path.join(parent, entry)): - names.add(entry) - agents_dir = os.path.join(os.path.dirname(parent), 'agents') - if os.path.isdir(agents_dir): - for entry in os.listdir(agents_dir): - if entry.endswith('.agent.md'): - names.add(entry[:-len('.agent.md')]) - elif entry.endswith('.md'): - names.add(entry[:-len('.md')]) +def _is_fs_root(path): + return os.path.dirname(path) == path - # Walk up looking for a monorepo root (plugins/*/.apm/) or a plugin root - # (.apm/). Capped so a pathological path can't turn this into a filesystem - # crawl; ten levels covers every real layout by a wide margin. + +def _collect_package(pkg_dir, names): + """Add every skill/agent name a package directory exposes, any layout.""" + for sub in ('.apm/skills/*/', 'skills/*/'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + names.add(os.path.basename(path.rstrip('/')).lower()) + for sub in ('.apm/agents/*.md', 'agents/*.md'): + for path in glob.glob(os.path.join(pkg_dir, sub)): + base = os.path.basename(path) + if base.endswith('.agent.md'): + base = base[:-len('.agent.md')] + else: + base = base[:-len('.md')] + names.add(base.lower()) + + +def _apm_package_root(start_dir): + """Nearest ancestor that is an apm package root (apm.yml or .apm/). + + The filesystem root is never a candidate: a stray /.apm/skills/ — a + scaffolding test's leftover, say, and one really does exist on at least one + machine here — would otherwise become the package root of every path on it. + Capped at ten levels so a pathological path can't become a filesystem + crawl; that covers every real layout by a wide margin. + """ current = os.path.abspath(start_dir) for _ in range(10): - # Never glob the filesystem root: a stray /.apm/skills/ (a scaffolding - # test's leftover, say) would otherwise become part of every skill's - # resolution universe on that machine. - if os.path.dirname(current) == current: - break - for pattern in ('plugins/*/.apm/skills/*/', '.apm/skills/*/'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path.rstrip('/'))) - for pattern in ('plugins/*/.apm/agents/*.agent.md', '.apm/agents/*.agent.md'): - for path in glob.glob(os.path.join(current, pattern)): - names.add(os.path.basename(path)[:-len('.agent.md')]) + if _is_fs_root(current): + return None + if (os.path.isfile(os.path.join(current, 'apm.yml')) + or os.path.isdir(os.path.join(current, '.apm'))): + return current current = os.path.dirname(current) + return None + + +def _authoring_root(start_dir): + """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + + Two passes, not one interleaved walk: a nested .git (a submodule, a + worktree of a sub-package) must not win over a real plugins/ root further + up. Both passes stop before the filesystem root for the same reason + _apm_package_root does. + """ + for probe in ( + lambda d: bool(glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(d, 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))): + current = os.path.abspath(start_dir) + for _ in range(12): + if _is_fs_root(current): + break + if probe(current): + return current + current = os.path.dirname(current) + return None + + +def _collect_authoring_root(root, names): + """Every plugin in the monorepo contributes its names.""" + for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + if os.path.isdir(pkg): + _collect_package(pkg, names) + + +def _declared_dependency_dirs(pkg_dir): + """Directories of the apm packages pkg_dir's manifest DECLARES. + + Reads dependencies.apm and resolves each entry to a directory on disk: + a monorepo-relative `path:` (against the package root and the nearest + ancestor manifest, which is the monorepo root) or an installed + apm_modules//. Entries that resolve to nothing are skipped — an + undeployed dependency contributes no names rather than an error. + """ + manifest = os.path.join(pkg_dir, 'apm.yml') + if not os.path.isfile(manifest): + return [] + try: + data = yaml.safe_load(read_text(manifest)) or {} + except Exception: + return [] + if not isinstance(data, dict): + return [] + deps = data.get('dependencies') + deps = deps.get('apm') if isinstance(deps, dict) else None + if not isinstance(deps, list): + return [] + + roots = [pkg_dir] + ancestor = os.path.dirname(os.path.abspath(pkg_dir)) + for _ in range(10): + if _is_fs_root(ancestor): + break + if os.path.isfile(os.path.join(ancestor, 'apm.yml')): + roots.append(ancestor) + break + ancestor = os.path.dirname(ancestor) + + found = [] + for entry in deps: + candidates = [] + if isinstance(entry, dict): + rel = entry.get('path') + name = entry.get('name') + if not name and rel: + name = os.path.basename(str(rel).rstrip('/')) + if rel: + candidates.extend(os.path.join(r, str(rel)) for r in roots) + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', str(name))) + elif isinstance(entry, str): + name = re.split(r'[#@]', entry)[0].strip().rstrip('/').split('/')[-1] + if name: + candidates.append(os.path.join(pkg_dir, 'apm_modules', name)) + candidates.extend(os.path.join(r, 'plugins', name) for r in roots) + for candidate in candidates: + if os.path.isdir(candidate): + found.append(candidate) + return found + + +def _deployed_roots(start_dir): + """.claude/ and .agents/ trees above start_dir — what a host really sees. + + Consulted ONLY when no authoring root exists; see the section header. The + filesystem root is skipped for the same reason _apm_package_root skips it: + a stray /.claude/skills/ must not join every path's universe. + """ + found = [] + current = os.path.abspath(start_dir) + for _ in range(10): + if _is_fs_root(current): + break + for name in ('.claude', '.agents'): + base = os.path.join(current, name) + if os.path.isdir(base): + found.append(base) + current = os.path.dirname(current) + return found + + +def known_targets(start_dir): + """Every skill/agent name a boundary clause in start_dir may name.""" + names = set() + start = os.path.abspath(start_dir) + + # Siblings: a cache-installed plugin and a deployed .claude/skills/ tree + # both put peers one level up, with no plugins/ directory above them. The + # grandparent is guarded against the filesystem root exactly like the two + # walk-up loops above — for a start dir of /skills/ the grandparent is + # `/`, and collecting there picks up this machine's stray /.apm/skills/. + parent = os.path.dirname(start) + grandparent = os.path.dirname(parent) + if (os.path.basename(parent) in ('skills', 'agents') + and os.path.isdir(parent) and not _is_fs_root(grandparent)): + _collect_package(grandparent, names) + + package = _apm_package_root(start) + if package: + _collect_package(package, names) + for dep_dir in _declared_dependency_dirs(package): + _collect_package(dep_dir, names) + + root = _authoring_root(start) + if root: + _collect_authoring_root(root, names) + else: + for base in _deployed_roots(start): + _collect_package(base, names) return names -NAME_ANY = r"[a-z0-9]+(?:-[a-z0-9]+)*" -NAME_HYPH = r"[a-z0-9]+(?:-[a-z0-9]+)+" +# --- Extraction ----------------------------------------------------------- +# False positives are the design constraint here, not recall. The rules: +# * A BARE target must be hyphenated AND sit in a boundary sentence (one +# carrying "do not"/"instead"/"rather than"/"not for"). Without the second +# condition, pc-run's "run pre-commit hooks" reads as a route to a +# non-existent `pre-commit` skill. +# * A BARE arrow target counts only in ADR-0020's compressed boundary form, +# `Not -> `. Without that, diagnose's process chain +# "fix -> regression-test" reads as a route to `regression-test`. +# * A backticked hyphenated token counts only inside a boundary sentence. +# Unconditionally, `pre-push` or `commit-msg` in a TRIGGER clause is a hard +# FAIL with no escape hatch. Gating it costs nothing (measured over this +# corpus: 54 targets before and after); DELETING it costs 7 real targets +# across three gitea skills, so it is gated, not removed. +# * SINGLE-WORD targets are deliberately NOT matchable bare — `research`, +# `triage`, `forge`, `prototype` and `tdd` are all real skill names and all +# ordinary English, so a bare-word rule would flag most of the corpus. A +# single-word target must be written `` `forge` `` or /forge to be seen. +# That is a known recall limitation, accepted over the false positives. +# Tool names (Read/Write/Edit) are excluded by the lowercase-only pattern; MCP +# tool names (issue_write) by its rejection of underscores; file names by its +# rejection of dots and slashes. +# +# ATTRIBUTIVE USE. The boundary-sentence gate above does NOT solve the +# `pre-commit` false positive, and the comment that claimed it did was wrong: +# "instead", "rather than", "do not" and "not for" are exactly the words a +# boundary clause uses, so the gate is open precisely where the risk is. All of +# these were hard dangling FAILs with no suppression: +# Use pre-commit hooks instead of ad-hoc scripts. +# Invoke the pull-request template instead of writing one by hand. +# Use conventional-commits formatting rather than free-form messages. +# Composes label-resolution logic instead of duplicating it. +# Do not use for X — run the `pre-push` hooks instead. +# What separates every one of them from a real route is grammar, not marking: +# the hyphenated token is a compound MODIFIER of the noun that follows it +# ("pre-commit hooks", "pull-request template"), where a route target is +# terminal — followed by punctuation, a conjunction, or a boundary word. So a +# target whose next token is an ordinary lowercase noun is CONFIRM-ONLY: it +# still resolves and still counts as a route when the name exists, but it can +# never raise a dangling error on its own. +# +# This is deliberately NOT the simpler "only marked targets may dangle" rule, +# which would have been wrong here: BOTH live true positives in this corpus are +# BARE — research's "(use neuledge-context)" and gitea-issues' "Composes +# gitea-labels-\n milestones", where the `>` fold yields "gitea-labels- +# milestones" and the trailing hyphen is what keeps it terminal. Marking is a +# poor proxy, so the follower token is the signal, and it is applied to +# backticked targets too. +# +# TERMINAL IS NOT ENOUGH — IN-SENTENCE CORROBORATION. The follower test clears +# `pre-push` in the example above only because that example happens to be +# followed by the noun "hooks". Move the same token into terminal position and +# it was a hard FAIL again, with no suppression mechanism anywhere in this gate: +# Do not use for running hooks — run `pre-commit` instead. +# Do not use for the commit message — see `commit-msg`. +# Do not use for type errors — run `type-check` first. +# Instead, use `semantic-release`. +# Do not use for the old flow — use the clean-up instead. +# Do not run end-to-end, run unit-tests. +# Every one of those is grammatically identical to a genuinely broken route: +# "route verb + hyphenated name + terminal" is also exactly how prose cites a +# tool, a hook, a file format or an English compound. Nothing local separates +# them, and the skills most exposed are the ones this contract sends authors +# back to rewrite first — pc-run, pc-author, vale-run, vale-config and the apm-* +# family are all ABOUT hyphenated tools. +# +# So the confidence to BLOCK a commit comes from the sentence, not the token: a +# prose-form target may raise a hard error only when its own sentence names at +# least one OTHER target that RESOLVES. A routing sentence proves itself by +# routing somewhere real; a lone unresolvable name proves nothing. That is not a +# rule fitted to the fixtures — it is the shape of both live true positives, +# which sit beside `write-docs` and `gitea-labels-milestones` respectively, and +# it changes this corpus's verdict by exactly nothing. +# +# An uncorroborated unresolvable target is NOT discarded: every caller reports +# it at its SUGGESTION tier, naming the target. The finding stays visible on +# every run; only the power to block a commit is withdrawn, which is the part +# that had no escape hatch. +# +# EXPLICIT ROUTE NOTATION is exempt from corroboration and always blocks: +# ADR-0020's compressed arrow (`Not -> `) and Claude Code's +# invocation form (`/`). Neither is ever how English cites a tool — nobody +# writes `-> pre-commit` or `/pre-commit` to mean the hook — so there is no +# ambiguity to resolve, and an author who wants a route checked unconditionally +# has two ways to say so. +# +# NAMESPACE: `plugin:skill` is live in this repo (native user-scope installs +# still resolve `gitea:gitea-prs`), so the patterns admit an optional +# `:` prefix and normalize_target() strips it before resolution. +NS = r"(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?" +NAME_ANY = NS + r"[a-z0-9]+(?:-[a-z0-9]+)*" +NAME_HYPH = NS + r"[a-z0-9]+(?:-[a-z0-9]+)+" ROUTE_VERB = (r"(?:use|uses|using|run|runs|invoke|invokes|invoking|try|see" - r"|that'?s|compose|composes|call|calls)") -# A "marked" target is unambiguous on its own: backticked (`git-commits`) or -# slash-command form (/skill-improve). A bare target is just a hyphenated word -# and is only read as a routing target inside a boundary sentence — otherwise -# "run pre-commit hooks" would be reported as a dangling route to `pre-commit`. + r"|that'?s|compose|composes|call|calls" + r"|routes?\s+to|delegates?\s+to|prefers?|switch(?:es)?\s+to" + r"|hands?\s+off\s+to)") MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) -# ADR-0020's compressed boundary form, `Not -> .`, with a -# bare target. Anchored on "not" so diagnose's process arrow chain -# ("fix -> regression-test") is not mistaken for a route. ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +# A boundary clause takes two shapes and BOTH count: the prose markers, and +# ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) SENTENCE_SPLIT = re.compile(u'(?<=[.!?])\\s+(?=[A-Z"“(])') -# Tool names (Read/Write/Edit) are excluded by construction: the name pattern is -# lowercase-only. MCP tool names (issue_write, pull_request_write) are excluded -# by construction too: the pattern admits no underscores. File names are -# excluded because the pattern admits no dots or slashes inside the name. +# The token that may follow a route target without turning it into a compound +# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a +# head noun that names the artifact itself ("the git-workflow skill"). Anything +# else — `hooks`, `template`, `formatting`, `logic` — is attributive prose. +FOLLOWER = re.compile(r"[`\s]*([a-z][a-z0-9]*)") +FOLLOWER_OK = frozenset(""" +and or nor but for to when if unless while after before with from in on at by +of as than then instead rather directly first only always never also even +both either neither so because since per via plus alone here there this that +these those it its they them is are was were be been being has have had will +would can could should must may might does do did +skill skills agent agents plugin plugins command commands +""".split()) -def _first(groups): - for g in groups: - if g: - return g - return None +def normalize_target(target): + """Comparison key: namespace stripped, lowercased. + + Extraction is case-insensitive (re.I) but the universe is built from + lowercase directory names, so `Git-Commits` at the start of a sentence + resolved to nothing until this normalization existed. + """ + return target.split(':')[-1].lower() + + +def has_boundary_clause(description): + return bool(BOUNDARY_MARKER.search(description) + or BOUNDARY_ARROW.search(description)) + + +def _first(match): + """(name, start, end) offsets for the first group that matched.""" + for index in range(1, (match.re.groups or 0) + 1): + if match.group(index): + return match.group(index), match.start(index), match.end(index) + return None, None, None + + +def _terminal(text, pos): + """True if the token at pos does not make the preceding name a modifier.""" + follower = FOLLOWER.match(text, pos) + return not follower or follower.group(1) in FOLLOWER_OK + + +def _notation(text, start, arrow): + """True if the name is written in route NOTATION rather than in prose. + + Two forms qualify: `/name` (Claude Code's invocation syntax, detected from + the character before the name) and `-> name` (ADR-0020's compressed boundary + form, passed in by the caller that matched the arrow). A backticked name + does NOT qualify — a code span is how a tool, a file and a skill are all + cited, so it carries no intent the follower test hasn't already read. + """ + return arrow or (start > 0 and text[start - 1] == '/') + + +def _add(out, text, name, start, end, strict=None, arrow=False): + if not name: + return + out.append((name, + _terminal(text, end) if strict is None else strict, + _notation(text, start, arrow))) def _scan(text, route_re, cont_re, out): - for m in route_re.finditer(text): - name = _first(m.groups()) + for match in route_re.finditer(text): + name, start, end = _first(match) if not name: continue - out.append(name) + _add(out, text, name, start, end) # "use git-history or git-branches instead" / "use gitea-issues / # gitea-prs" — keep consuming conjoined targets after the first. - pos = m.end() + pos = match.end() while True: - cm = cont_re.match(text, pos) - if not cm: + cont = cont_re.match(text, pos) + if not cont: break - nxt = _first(cm.groups()) - if nxt: - out.append(nxt) - pos = cm.end() + _add(out, text, *_first(cont)) + pos = cont.end() -def boundary_targets(desc): +def _extract_sentence(sentence): + """[(name, may_dangle, notation)] for the routing targets in ONE sentence. + + Kept separate from _extract() because corroboration is scoped to a single + sentence: a target's evidence is what stands beside it, not what the rest of + the description happens to mention. + """ out = [] - for sentence in SENTENCE_SPLIT.split(desc): - boundary = bool(BOUNDARY_MARKER.search(sentence)) - _scan(sentence, - ROUTE_ANY if boundary else ROUTE_MARKED, - CONT_ANY if boundary else CONT_MARKED, - out) - for m in ARROW_MARKED.finditer(sentence): - name = _first(m.groups()) - if name: - out.append(name) - for m in ARROW_BOUNDARY.finditer(sentence): - out.append(m.group(1)) - out.extend(BACKTICK.findall(sentence)) - return sorted(set(out)) + boundary = bool(BOUNDARY_MARKER.search(sentence)) + _scan(sentence, + ROUTE_ANY if boundary else ROUTE_MARKED, + CONT_ANY if boundary else CONT_MARKED, + out) + for match in ARROW_MARKED.finditer(sentence): + # `-> name` and `-> /name` are route notation, not prose: nothing + # reads as a compound modifier after an arrow, so no follower test. + _add(out, sentence, *_first(match), strict=True, arrow=True) + for match in ARROW_BOUNDARY.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1), + strict=True, arrow=True) + if boundary: + for match in BACKTICK.finditer(sentence): + _add(out, sentence, match.group(1), match.start(1), match.end(1)) + return out + + +def _extract(description): + """[(name, may_dangle, notation)] for every routing target.""" + out = [] + for sentence in SENTENCE_SPLIT.split(description): + out.extend(_extract_sentence(sentence)) + return out + + +def boundary_targets(description): + """Every routing target, for reporting and for confirming a route.""" + return sorted({name for name, _, _ in _extract(description)}) + + +def unresolved_targets(description, known): + """Targets resolving to nothing, split into (blocking, reported). + + `blocking` earns a hard error; `reported` is SUGGESTION tier — named on + every run, never fatal. Three conditions gate the promotion, and all of them + are documented at length in the ATTRIBUTIVE USE and CORROBORATION notes + above: + + 1. the target must be terminal, not a compound modifier ("pre-commit + hooks" is prose about a tool, not a route), + 2. it must be written in route notation (`/name`, `-> name`), OR + 3. its own sentence must name another target that DOES resolve. + + Everything else is reported and left alone. `known` is the resolved + universe from known_targets(); passing an empty set is not meaningful — + callers check for that first and decline out loud instead. + """ + blocking, reported = set(), set() + for sentence in SENTENCE_SPLIT.split(description): + found = _extract_sentence(sentence) + resolved = {normalize_target(name) for name, _, _ in found + if normalize_target(name) in known} + for name, may_dangle, notation in found: + key = normalize_target(name) + if key in known or not may_dangle: + continue + if notation or (resolved - {key}): + blocking.add(name) + else: + reported.add(name) + return sorted(blocking), sorted(reported - blocking) + +# --- Frontmatter ---------------------------------------------------------- +# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading +# blank line, trailing whitespace after either `---`, or CRLF line endings all +# defeated the old `^---\n(.*?)\n---`, and the miss was SILENT: every ADR-0020 +# check was skipped and the file reported green (measured: a 550-character +# description with a 1,000-word body exited 0 behind a BOM). A file that cannot +# be measured must never report green, so every caller of these two ERRORs on a +# miss instead of moving on. +FRONTMATTER_RE = re.compile( + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + + +def strip_bom(text): + return text[1:] if text.startswith(u'') else text + + +class FrontmatterError(Exception): + pass + + +def description_value(fm_text): + """The description VALUE, with YAML folding resolved. + + PyYAML is a HARD requirement, preflighted in bash. The hand-rolled fallback + this replaced diverged from a real parser across the FAIL boundary — one + corpus description measured 270 characters parsed and 412 unparsed, and a + quoted `"description"` key or an explicit `description: null` returned empty + from it, silently skipping the description AND routing checks. A gate that + disagrees with itself depending on which reader ran is worse than no gate. + + This is the ONLY reader any of the three scripts may use to decide whether a + description is present. A line regex cannot: `description:` with no value + followed by `model: sonnet` lets `\\s*` cross the newline and captures the + NEXT key, which reads as a non-empty description, skips the "missing or + empty" failure, and then early-returns out of every ADR-0020 gate on the + genuinely empty folded value. That combination exited 0 with zero output on + a BLOCKING pre-push gate. + """ + try: + data = yaml.safe_load(fm_text) + except Exception as exc: + raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + if not isinstance(data, dict): + raise FrontmatterError('frontmatter is not a YAML mapping') + value = data.get('description') + if value is None: + return '' + if not isinstance(value, str): + value = str(value) + return re.sub(r'\s+', ' ', value).strip() + + +# --- Body-shape checks (skills only; agents have no references/ dir) ------- +# Deterministic and countable, so they are enforced here. Whether a given +# gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why +# both gotcha checks are SUGGESTION tier. A missing reference file is not a +# style opinion — it is a broken pointer — so that one is ERROR tier. +# +# Both read a FENCE-MASKED copy of the body. Scanning the raw body made a +# ```-fenced example a hard ERROR — and the skills most likely to carry one are +# skill-author and skill-audit, which DOCUMENT the references/ convention — and +# let a `## Gotchas` heading inside a fenced block stand in for the real +# section. Masking preserves every byte offset (content becomes spaces, +# newlines stay), so a span found in the mask slices the original. +GOTCHA_MAX_ENTRIES = 5 +GOTCHA_MAX_BODY_FRACTION = 0.25 +# The heading has to BE "Gotchas", not merely contain the word: `## Gotcha +# handling` and `## Why gotchas matter` are prose sections, and treating one as +# the Gotchas section measured a span that was never a gotcha list. +GOTCHA_HEADING = re.compile(r'^(#{1,6})[ \t]+(?:[^\n]*?[ \t])?gotchas?[ \t]*:?[ \t]*$', + re.I | re.M) +# Column 0 only. `^[ \t]{0,3}` counted a two-space-indented CHILD bullet as a +# top-level entry, so a five-entry section with sub-bullets reported nine. +GOTCHA_ENTRY = re.compile(r'^(?:[-*+]|\d+[.)])[ \t]+', re.M) +FENCE_OPEN = re.compile(r'^[ \t]{0,3}(`{3,}|~{3,})') +REFERENCE_POINTER = re.compile( + r'(?= len(fence) + and not stripped.strip()[len(marker):].strip()): + fence = None + return ''.join(out) + + +def gotcha_stats(body): + """(entry count, section word count) for the first Gotchas section, or None. + + The section runs to the next heading at the same level or shallower. + Entries are top-level list items; a section written as subheadings instead + of a list counts those. Headings and entries are read from the fence mask; + the word count is taken from the original slice, because fenced lines are + real body words and the fraction is measured against the whole body. + """ + masked = mask_fenced(body) + match = GOTCHA_HEADING.search(masked) + if not match: + return None + level = len(match.group(1)) + rest = masked[match.end():] + nxt = re.search(r'^#{1,%d}[ \t]+' % level, rest, re.M) + end = match.end() + (nxt.start() if nxt else len(rest)) + section = masked[match.end():end] + entries = len(GOTCHA_ENTRY.findall(section)) + if entries == 0 and level < 6: + entries = len(re.findall(r'^#{%d,6}[ \t]+' % (level + 1), section, re.M)) + return entries, len(body[match.end():end].split()) + + +def missing_reference_pointers(body, skill_dir): + """references/.md named in the body but absent from disk.""" + masked = mask_fenced(body) + missing = set() + for match in REFERENCE_POINTER.finditer(masked): + start = masked.rfind('\n', 0, match.start()) + 1 + end = masked.find('\n', match.end()) + if end < 0: + end = len(masked) + if REFERENCE_PAST.search(masked[start:end]): + continue + if REFERENCE_QUALIFIER.search(masked[start:match.start()]): + continue + if not os.path.isfile(os.path.join(skill_dir, 'references', match.group(1))): + missing.add('references/' + match.group(1)) + return sorted(missing) +# ===== END ADR-0020 SHARED BOUNDARY RESOLVER ===== # --- Per-file checks ------------------------------------------------------ for path in files: if not os.path.isfile(path): + # NOT a silent skip. A broken symlink named SKILL.md is storable in git, + # so pre-commit really can hand one to this hook, and a directory named + # SKILL.md reaches it the same way — both used to exit 0 with zero + # output, which is precisely the "stay quiet about a measurement it did + # not take" failure this script forbids itself two screens up. + if os.path.isdir(path): + why = "is a directory, not a file" + elif os.path.islink(path): + why = "is a symlink that does not resolve to a file" + elif os.path.exists(path): + why = "is not a regular file" + else: + why = "does not exist" + error("%s: %s, so none of the ADR-0020 gates could run on it. A path " + "this gate was handed and could not read does not get to pass in " + "silence." % (path, why)) + continue + try: + content = strip_bom(read_text(path)) + except EncodingError as exc: + error("%s: %s. None of the ADR-0020 gates could run on this file." + % (path, exc)) continue - with open(path) as fh: - content = fh.read() - fm_match = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) + fm_match = FRONTMATTER_RE.match(content) if not fm_match: - # A missing frontmatter block is skill-frontmatter's / validate.sh's - # finding, not this hook's — nothing to measure, so say nothing. + error("%s: no parseable YAML frontmatter block. Expected a `---` line, " + "the fields, then a closing `---` line (a BOM, leading blank " + "lines, trailing spaces after either marker and CRLF endings are " + "all tolerated). None of the ADR-0020 gates could run on this " + "file — a file that cannot be measured does not get to pass." + % path) + continue + + try: + desc = description_value(fm_match.group(1)) + except FrontmatterError as exc: + error("%s: frontmatter is not valid YAML (%s). None of the ADR-0020 " + "gates could run on this file." % (path, exc)) continue - desc = extract_description(fm_match.group(1)) body = content[fm_match.end():] + skill_dir = os.path.dirname(os.path.abspath(path)) + + # An absent or empty description is an ERROR here too, not a silent skip. + # All three ADR-0020 scripts have to agree on this input: the description is + # the one field preloaded into every session, so a SKILL.md that ships + # without one is the worst case the contract exists for, and a gate that + # merely declines to measure it reports green. + if not desc: + error("%s: description field is missing or empty. It is the only part of a " + "skill preloaded into every session, so a skill without one can never " + "be routed to — and none of the ADR-0020 description or boundary gates " + "have anything to measure." % path) if desc: dlen = len(desc) @@ -368,25 +931,67 @@ for path in files: suggest("%s: body is %d words, over the %d-word target (ADR-0020, hard fail at %d)." % (path, body_words, BODY_SUGGEST_WORDS, BODY_MAX_WORDS)) + # Reference pointers must exist. ERROR, not SUGGESTION: a dispatch table + # naming a file that is not on disk is a hard break, and nothing else in + # the gate/audit/vale stack notices it. + for ref in missing_reference_pointers(body, skill_dir): + error("%s: body points at %s, which does not exist on disk. A dispatch " + "table or \"read X\" trigger naming a missing file sends the " + "agent nowhere." % (path, ref)) + + # Gotchas discipline. SUGGESTION on both counts: the measurement is + # deterministic, but whether a given gotcha earns its place is judgment. + stats = gotcha_stats(body) + if stats is not None: + entries, section_words = stats + if entries > GOTCHA_MAX_ENTRIES: + suggest("%s: Gotchas section has %d entries, over the %d-entry guideline. " + "A list that long is usually a missing references/ file or a design " + "problem written up as a warning." + % (path, entries, GOTCHA_MAX_ENTRIES)) + if body_words and section_words > body_words * GOTCHA_MAX_BODY_FRACTION: + suggest("%s: Gotchas section is %d of %d body words (%d%%), over the %d%% " + "guideline. Move the durable parts to references/ and keep the " + "section for live traps." + % (path, section_words, body_words, + round(100.0 * section_words / body_words), + round(100.0 * GOTCHA_MAX_BODY_FRACTION))) + + # Missing boundary clause. SUGGESTION, not ERROR: detecting the absence is + # deterministic, but whether this particular skill warrants one is the + # auditor's call. Both accepted shapes count — the prose markers and + # ADR-0020's compressed `Not -> ` arrow. + if desc and not has_boundary_clause(desc): + suggest("%s: description has no boundary clause (ADR-0020). Add the prose form " + "(\"Do not use for X — use `y` instead\") or the compressed form " + "(\"Not X -> y\") so the router knows where NOT to send this skill." + % path) + targets = boundary_targets(desc) if targets: - # Two roots, unioned: the file's own directory (which finds siblings in - # a cache install and walks up to a monorepo root in a checkout), and - # this script's own repo root (which is authoritative when pre-commit - # hands over a relative path from an unrelated cwd). - known = known_targets(os.path.dirname(os.path.abspath(path))) - known |= known_targets(repo_root) - # An empty universe means the resolver found no authoring source at all - # (a SKILL.md audited outside any plugin tree). Reporting every target - # as dangling there would be noise, not a finding. + known = known_targets(skill_dir) if known: - for target in targets: - if target not in known: - error("%s: description routes to '%s', which does not resolve to a skill " - "under plugins/*/.apm/skills/ or an agent under " - "plugins/*/.apm/agents/ (ADR-0020). A boundary clause that names a " - "non-existent target sends the router nowhere." - % (path, target)) + blocking, reported = unresolved_targets(desc, known) + for target in blocking: + error("%s: description routes to '%s', which does not resolve to a skill " + "or agent in this monorepo, in this package, or in a package it " + "declares in apm.yml dependencies.apm (ADR-0020). A boundary clause " + "that names a non-existent target sends the router nowhere." + % (path, target)) + for target in reported: + suggest("%s: description routes to '%s', which does not resolve to a skill " + "or agent in this monorepo, in this package, or in a package it " + "declares in apm.yml dependencies.apm (ADR-0020). SUGGESTION rather " + "than a hard failure because nothing else in the sentence resolves, " + "so this is equally likely to be a tool, a file format or an English " + "compound. If it IS a route, write it as `/%s` or `-> %s` and it will " + "be checked properly." % (path, target, target, target)) + else: + info("%s: boundary-target resolution DID NOT RUN — no skill universe " + "could be determined for this path (no authoring root above it, no " + "apm package root, no declared apm dependencies, no deployed " + ".claude/ or .agents/ tree). Unchecked target(s): %s" + % (path, ", ".join(targets))) sys.exit(1 if failed else 0) PYTHON diff --git a/tests/test-adr0020-body-checks.sh b/tests/test-adr0020-body-checks.sh new file mode 100755 index 0000000..b1026ad --- /dev/null +++ b/tests/test-adr0020-body-checks.sh @@ -0,0 +1,406 @@ +#!/usr/bin/env bash +# Regression test for the ADR-0020 body-shape checks and, just as importantly, +# for the false-positive fixes each of them needed. Every check here was +# completely untested. +# +# * Gotchas section over 5 entries — SUGGESTION +# * Gotchas section over 25% of the body — SUGGESTION +# * a references/.md named but absent — ERROR (a broken pointer is not a +# style opinion) +# * description with no boundary clause — SUGGESTION +# +# The false-positive half is not optional extra coverage. Each of these checks +# scans prose, and the first naive version of each one fired on ordinary writing: +# a ```-fenced EXAMPLE of a Gotchas section became the section itself, indented +# child bullets were counted as top-level entries, `## Gotcha handling` was read +# as the Gotchas section, and a documented-then-removed references/ file became a +# hard ERROR. The skills most likely to carry such an example are skill-author and +# skill-audit — the two that DOCUMENT these conventions — so a gate that fires on +# them is a gate nobody can turn on. +# +# Every case is a matched pair: the check fires just over its boundary, and stays +# silent just under it (or on the shape it must not match). A test asserting only +# that a bad file fails proves nothing about a check that fires on everything. +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +HOOK="$REPO_ROOT/scripts/skill-size-check.sh" +PASS=0 +FAIL=0 + +pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } +fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } + +TMPDIR_T="$(mktemp -d)" +trap 'rm -rf "$TMPDIR_T"' EXIT + +# A description with a boundary clause and no routing target — the neutral +# default, so a fixture about Gotchas or references does not also trip the +# missing-boundary-clause SUGGESTION and stop isolating what it names. +CLEAN_DESC="Use when doing the thing. Do not use for anything else." + +# make_skill — SKILL.md with the body read from stdin. Echoes the +# path. Each skill gets its own directory so references/ fixtures are isolated. +make_skill() { + local name="$1" desc="$2" dir + dir="$TMPDIR_T/$name" + mkdir -p "$dir" + { + echo "---" + echo "name: $name" + echo "description: $desc" + echo "---" + cat + } > "$dir/SKILL.md" + echo "$dir/SKILL.md" +} + +# expect