From f7cc27908c00f847a19cdca7fdaaea42eb8596b4 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Sun, 16 Aug 2026 19:48:56 +0000 Subject: [PATCH] fix(kyberforge): close the vacuous-green and consumer-resolution defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the ADR-0020 gate found four ways it could exit 0 without measuring, and one way it hard-failed a repo it had no business failing. On a gate shipping hot with no baseline, a silent pass is the worst outcome available and a false block is the second worst. Consumer resolution was the blocker. _authoring_root() fell back to the nearest .git, so it returned truthy in ANY git repo; _collect_authoring_root() then contributed nothing and the deployed-tree branch was dead code in precisely the consumer case it exists for. A consumer repo routing to an installed sibling got an unblockable ERROR, and deleting .git "fixed" it. It now keys on which of the two walk-up passes matched. A name-count delta was tried first and is wrong: a single-plugin monorepo re-collects its own package and adds no new name, so the delta reads zero and drags the deployed trees — including a global ~/.claude — back into the universe. That reintroduces the install-dependence ADR-0020 forbids, one layer down. The three silent passes: an indented `---` inside a block scalar truncated the frontmatter and reclassified the rest of the description as body; a non-string description was str()-coerced, so `description: true` measured as the four-character "True"; and an unterminated fence blanked the rest of the body, disabling the ERROR-tier references/ check and the gotcha counts. Two measurement defects came with them. The awk line/word counts discarded awk's exit status, so an unreadable file passed both spec ceilings in total silence, and awk NR/NF disagreed with the audit script's splitlines()/split() on Unicode whitespace — the "fix one gate, get blocked by the other" bug, on the two axes the differential test deliberately excluded. Both counts now run in the Python block that already reads the file. A type error also no longer reports itself as a syntax error. Also: glob metacharacters in the checkout path silently disabled the resolver; re.I was applied to some extraction patterns and not others; agent-audit missed `tools:` written as a YAML block sequence, the shape Copilot files use; and a nonexistent agent file raised a bare FileNotFoundError instead of a diagnostic. The shared resolver block stays byte-identical across all three scripts. Corpus output is unchanged — 26 description FAIL, 9 body FAIL, 2 dangling, 0 missing references, 58 SUGGESTIONs — so no documented count moves. Refs: #99 ADR: 0020 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015W3iwF9ncfRZddGBxsMCYi --- .../skills/agent-audit/scripts/validate.sh | 178 ++++++++++++---- .../skills/skill-audit/scripts/validate.sh | 132 +++++++++--- .../skills/agent-audit/scripts/validate.sh | 178 ++++++++++++---- .../skills/skill-audit/scripts/validate.sh | 132 +++++++++--- scripts/skill-size-check.sh | 196 +++++++++++++----- 5 files changed, 627 insertions(+), 189 deletions(-) diff --git a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh index 5738db4..2e1882f 100755 --- a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh @@ -222,17 +222,21 @@ def read_text(path): # 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 .claude/ and .agents/ trees are deliberately NOT consulted when the +# root came from the plugins/ probe. 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. +# Deployed trees ARE used when no plugin monorepo was found — whether the walk +# landed on a bare .git ancestor or on nothing at all. That is the consumer +# case: the file being checked lives in or beside a deployed tree, inside an +# ordinary git repo, with no monorepo to read. The two cases are told apart by +# which probe matched, never by how many names a root contributed; see +# known_targets(). def _is_fs_root(path): @@ -241,11 +245,17 @@ def _is_fs_root(path): def _collect_package(pkg_dir, names): """Add every skill/agent name a package directory exposes, any layout.""" + # glob.escape() the DIRECTORY only. A checkout path containing `[`, `]`, + # `*` or `?` — a worktree named `feature[2]`, say — otherwise turns the + # whole pattern into a character class that matches nothing, and the + # resolver degrades to the "DID NOT RUN" INFO with rc=0 across every file + # in the tree. The wildcards in `sub` are the intended ones and stay raw. + safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): - for path in glob.glob(os.path.join(pkg_dir, sub)): + for path in glob.glob(os.path.join(safe_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)): + for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) if base.endswith('.agent.md'): base = base[:-len('.agent.md')] @@ -277,28 +287,35 @@ def _apm_package_root(start_dir): def _authoring_root(start_dir): """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + Returns (root, matched_plugins_probe). The flag reports WHICH probe + matched: True for the plugins/*/.apm/{skills,agents} glob, False for the + .git fallback and for no match at all. known_targets() needs that + distinction — only a real plugins/ root makes the deployed trees + redundant, and a name-count delta cannot tell the two apart. + 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'))): + probes = ( + lambda d: bool(glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))) + for index, probe in enumerate(probes): current = os.path.abspath(start_dir) for _ in range(12): if _is_fs_root(current): break if probe(current): - return current + return current, index == 0 current = os.path.dirname(current) - return None + return None, False def _collect_authoring_root(root, names): """Every plugin in the monorepo contributes its names.""" - for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + for pkg in glob.glob(os.path.join(glob.escape(root), 'plugins', '*')): if os.path.isdir(pkg): _collect_package(pkg, names) @@ -362,7 +379,7 @@ def _declared_dependency_dirs(pkg_dir): 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 + Consulted ONLY when no plugin monorepo root was found; see the 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. """ @@ -401,10 +418,22 @@ def known_targets(start_dir): for dep_dir in _declared_dependency_dirs(package): _collect_package(dep_dir, names) - root = _authoring_root(start) + # A .git ancestor is an authoring root only if it actually holds plugins. + # _authoring_root() falls back to the nearest .git, so it is truthy in ANY + # git repo; without the distinction that fallback wins in every consumer + # checkout, _collect_authoring_root() contributes nothing, and the deployed + # branch below is dead code in the exact case it exists for. So condition + # on WHICH probe matched, which _authoring_root() reports directly. A + # name-count delta looks equivalent and is not: _collect_authoring_root() + # re-collects the checked file's own plugin, whose names the blocks above + # already added, so a one-plugin monorepo shows a delta of zero and would + # wrongly reach for the deployed trees — including the user's global + # ~/.claude/skills, making the verdict depend on what happens to be + # installed (ADR-0020 lines 118-127). + root, root_has_plugins = _authoring_root(start) if root: _collect_authoring_root(root, names) - else: + if not root_has_plugins: for base in _deployed_roots(start): _collect_package(base, names) return names @@ -510,11 +539,17 @@ MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +# re.I on ALL of them, uniformly. The patterns are built from the same +# lowercase NAME_* fragments, so half of them carrying the flag and half not +# meant `Skill-Audit` at the start of a boundary sentence was extracted by +# ROUTE_ANY but invisible to BACKTICK — contradicting normalize_target()'s own +# docstring, which exists precisely because extraction is case-insensitive and +# the universe is not. +CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) +CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) +ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # 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) @@ -681,8 +716,16 @@ def unresolved_targets(description, known): # 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. +# +# The CLOSING marker is anchored at column 0 — deliberately NOT `[ \t]*---`. +# YAML block-scalar content must be indented deeper than its key, so an +# indented `---` inside a folded description is CONTENT; letting it close the +# frontmatter truncated the description mid-value and silently reclassified the +# rest as body, which is a vacuous green in both directions at once. Leading +# whitespace is still tolerated on the OPENING marker, where no such content +# can exist. FRONTMATTER_RE = re.compile( - r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)', re.DOTALL) def strip_bom(text): @@ -714,14 +757,26 @@ def description_value(fm_text): try: data = yaml.safe_load(fm_text) except Exception as exc: - raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + # Every FrontmatterError message is a COMPLETE clause, never a detail a + # caller wraps in one. Callers used to prefix a hard-coded "frontmatter + # is not valid YAML (...)", which is true only of this branch: the two + # type failures below come from frontmatter that parsed fine, and + # telling their author the YAML is invalid sends them hunting for a + # syntax error that is not there — on a blocking gate with no baseline. + raise FrontmatterError('frontmatter is not valid YAML (%s)' + % 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) + # NOT str()-coerced. `description: true` became the 4-character "True" + # and sailed through the 400-character gate; a list or mapping was + # measured as its Python repr. Neither is a description a host can + # preload, so this is a parse failure, reported as one. + raise FrontmatterError( + 'description is a %s, not a string' % type(value).__name__) return re.sub(r'\s+', ' ', value).strip() @@ -791,6 +846,18 @@ def mask_fenced(text): if (marker and marker[0] == fence[0] and len(marker) >= len(fence) and not stripped.strip()[len(marker):].strip()): fence = None + # An UNCLOSED fence has no cost-free answer, only a choice of which way to + # be wrong. Masking to end-of-body blanks the rest of the body, silently + # disabling the ERROR-tier references/ check and the gotcha counts. + # Returning the raw text instead exposes the unclosed example's own + # content, so a fenced example naming a nonexistent references/ file + # becomes a hard ERROR it would not have been had the fence been closed — + # confirmed, not hypothetical. The loud-false-positive direction is the one + # chosen: this script's rule is that a file it cannot measure must never + # report green, and masking-onward is exactly that failure. Both outcomes + # need an already-malformed file, and the false positive costs one fence. + if fence is not None: + return text return ''.join(out) @@ -869,12 +936,15 @@ def get_frontmatter_keys(fm): return keys def agent_description(fm, local_fname): - """The folded description VALUE, or None if the frontmatter is not YAML.""" + """The folded description VALUE, or None if it could not be read.""" try: 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}") + # `exc` carries the whole clause — invalid YAML, a non-mapping block, or + # a description of the wrong type. Do not prefix a diagnosis here; the + # last one named a syntax error for two failures that have none. + fail(f"{exc} — the ADR-0020 description and boundary-target gates could " + f"not run — {local_fname}") return None def check_description_budget(value, local_fname): @@ -944,11 +1014,31 @@ def check_boundary(value, fpath, local_fname): 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') - if not val: + """Tool names from the `tools` field — inline scalar OR YAML block sequence. + + Read off the PARSED mapping, never off extract_field(). That function's + capture is newline-bounded on purpose (`[^\\S\\r\\n]*(.+)`), so a `tools:` + written as a block sequence — the shape Copilot agent files use — captured + nothing at all and the subagent-unavailable-tool check silently stopped + firing on exactly the files it was written for. Both spellings are legal + YAML, so both are read here. + """ + try: + data = yaml.safe_load(fm) + except Exception: + # Not this function's failure to report: the frontmatter's validity is + # decided (and failed) by agent_description() on the same text. return set() - return set(re.split(r'[\s,]+', val.strip())) + if not isinstance(data, dict): + return set() + val = data.get('tools') + if isinstance(val, list): + items = [str(item).strip() for item in val] + elif isinstance(val, str): + items = re.split(r'[\s,]+', val.strip()) + else: + return set() + return {item for item in items if item} def is_copilot_cloud_ide(fpath): """True if the file is a cloud/IDE Copilot agent (name is optional for these).""" @@ -1063,6 +1153,15 @@ def check_apm_agent_file(fpath, allowlist, stem): fail(f"file is {exc}. Nothing could be measured, so this is a hard " f"failure, not a skip — {local_fname}") return + except OSError as exc: + # A path that cannot be opened gets a FAIL line naming it, not a bare + # FileNotFoundError traceback. scripts/check-apm-agents-valid.sh takes + # this path for an agent file deleted from the worktree but still + # tracked in the index — a real, expected state, and the caller needs to + # be told which file, not handed an interpreter stack. + fail(f"could not be read ({exc.strerror or exc}): {fpath}. Nothing could " + f"be measured, so this is a hard failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: @@ -1168,6 +1267,13 @@ def check_file(fpath, file_provider): fail(f"file is {exc}. Nothing could be measured, so this is a hard " f"failure, not a skip — {local_fname}") return + except OSError as exc: + # Same reason as check_apm_agent_file's: a diagnostic naming the path + # beats a FileNotFoundError traceback. The counterpart is pre-checked at + # the bottom of this script, but agent_file itself never was. + fail(f"could not be read ({exc.strerror or exc}): {fpath}. Nothing could " + f"be measured, so this is a hard failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: diff --git a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh index 35c9733..f80f02f 100755 --- a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh @@ -148,17 +148,21 @@ def read_text(path): # 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 .claude/ and .agents/ trees are deliberately NOT consulted when the +# root came from the plugins/ probe. 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. +# Deployed trees ARE used when no plugin monorepo was found — whether the walk +# landed on a bare .git ancestor or on nothing at all. That is the consumer +# case: the file being checked lives in or beside a deployed tree, inside an +# ordinary git repo, with no monorepo to read. The two cases are told apart by +# which probe matched, never by how many names a root contributed; see +# known_targets(). def _is_fs_root(path): @@ -167,11 +171,17 @@ def _is_fs_root(path): def _collect_package(pkg_dir, names): """Add every skill/agent name a package directory exposes, any layout.""" + # glob.escape() the DIRECTORY only. A checkout path containing `[`, `]`, + # `*` or `?` — a worktree named `feature[2]`, say — otherwise turns the + # whole pattern into a character class that matches nothing, and the + # resolver degrades to the "DID NOT RUN" INFO with rc=0 across every file + # in the tree. The wildcards in `sub` are the intended ones and stay raw. + safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): - for path in glob.glob(os.path.join(pkg_dir, sub)): + for path in glob.glob(os.path.join(safe_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)): + for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) if base.endswith('.agent.md'): base = base[:-len('.agent.md')] @@ -203,28 +213,35 @@ def _apm_package_root(start_dir): def _authoring_root(start_dir): """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + Returns (root, matched_plugins_probe). The flag reports WHICH probe + matched: True for the plugins/*/.apm/{skills,agents} glob, False for the + .git fallback and for no match at all. known_targets() needs that + distinction — only a real plugins/ root makes the deployed trees + redundant, and a name-count delta cannot tell the two apart. + 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'))): + probes = ( + lambda d: bool(glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))) + for index, probe in enumerate(probes): current = os.path.abspath(start_dir) for _ in range(12): if _is_fs_root(current): break if probe(current): - return current + return current, index == 0 current = os.path.dirname(current) - return None + return None, False def _collect_authoring_root(root, names): """Every plugin in the monorepo contributes its names.""" - for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + for pkg in glob.glob(os.path.join(glob.escape(root), 'plugins', '*')): if os.path.isdir(pkg): _collect_package(pkg, names) @@ -288,7 +305,7 @@ def _declared_dependency_dirs(pkg_dir): 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 + Consulted ONLY when no plugin monorepo root was found; see the 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. """ @@ -327,10 +344,22 @@ def known_targets(start_dir): for dep_dir in _declared_dependency_dirs(package): _collect_package(dep_dir, names) - root = _authoring_root(start) + # A .git ancestor is an authoring root only if it actually holds plugins. + # _authoring_root() falls back to the nearest .git, so it is truthy in ANY + # git repo; without the distinction that fallback wins in every consumer + # checkout, _collect_authoring_root() contributes nothing, and the deployed + # branch below is dead code in the exact case it exists for. So condition + # on WHICH probe matched, which _authoring_root() reports directly. A + # name-count delta looks equivalent and is not: _collect_authoring_root() + # re-collects the checked file's own plugin, whose names the blocks above + # already added, so a one-plugin monorepo shows a delta of zero and would + # wrongly reach for the deployed trees — including the user's global + # ~/.claude/skills, making the verdict depend on what happens to be + # installed (ADR-0020 lines 118-127). + root, root_has_plugins = _authoring_root(start) if root: _collect_authoring_root(root, names) - else: + if not root_has_plugins: for base in _deployed_roots(start): _collect_package(base, names) return names @@ -436,11 +465,17 @@ MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +# re.I on ALL of them, uniformly. The patterns are built from the same +# lowercase NAME_* fragments, so half of them carrying the flag and half not +# meant `Skill-Audit` at the start of a boundary sentence was extracted by +# ROUTE_ANY but invisible to BACKTICK — contradicting normalize_target()'s own +# docstring, which exists precisely because extraction is case-insensitive and +# the universe is not. +CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) +CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) +ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # 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) @@ -607,8 +642,16 @@ def unresolved_targets(description, known): # 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. +# +# The CLOSING marker is anchored at column 0 — deliberately NOT `[ \t]*---`. +# YAML block-scalar content must be indented deeper than its key, so an +# indented `---` inside a folded description is CONTENT; letting it close the +# frontmatter truncated the description mid-value and silently reclassified the +# rest as body, which is a vacuous green in both directions at once. Leading +# whitespace is still tolerated on the OPENING marker, where no such content +# can exist. FRONTMATTER_RE = re.compile( - r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)', re.DOTALL) def strip_bom(text): @@ -640,14 +683,26 @@ def description_value(fm_text): try: data = yaml.safe_load(fm_text) except Exception as exc: - raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + # Every FrontmatterError message is a COMPLETE clause, never a detail a + # caller wraps in one. Callers used to prefix a hard-coded "frontmatter + # is not valid YAML (...)", which is true only of this branch: the two + # type failures below come from frontmatter that parsed fine, and + # telling their author the YAML is invalid sends them hunting for a + # syntax error that is not there — on a blocking gate with no baseline. + raise FrontmatterError('frontmatter is not valid YAML (%s)' + % 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) + # NOT str()-coerced. `description: true` became the 4-character "True" + # and sailed through the 400-character gate; a list or mapping was + # measured as its Python repr. Neither is a description a host can + # preload, so this is a parse failure, reported as one. + raise FrontmatterError( + 'description is a %s, not a string' % type(value).__name__) return re.sub(r'\s+', ' ', value).strip() @@ -717,6 +772,18 @@ def mask_fenced(text): if (marker and marker[0] == fence[0] and len(marker) >= len(fence) and not stripped.strip()[len(marker):].strip()): fence = None + # An UNCLOSED fence has no cost-free answer, only a choice of which way to + # be wrong. Masking to end-of-body blanks the rest of the body, silently + # disabling the ERROR-tier references/ check and the gotcha counts. + # Returning the raw text instead exposes the unclosed example's own + # content, so a fenced example naming a nonexistent references/ file + # becomes a hard ERROR it would not have been had the fence been closed — + # confirmed, not hypothetical. The loud-false-positive direction is the one + # chosen: this script's rule is that a file it cannot measure must never + # report green, and masking-onward is exactly that failure. Both outcomes + # need an already-malformed file, and the false positive costs one fence. + if fence is not None: + return text return ''.join(out) @@ -802,8 +869,11 @@ name = name_m.group(1).strip('"\'') if name_m else "" 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") + # `exc` carries the whole clause — invalid YAML, a non-mapping block, or a + # description of the wrong type. Do not prefix a diagnosis here; the last + # one named a syntax error for two failures that have none. + fail(f"{exc}. Nothing downstream can be measured, so this is a hard " + f"failure, not a skip") print("One or more checks failed.") sys.exit(1) diff --git a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh index 5738db4..2e1882f 100755 --- a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh @@ -222,17 +222,21 @@ def read_text(path): # 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 .claude/ and .agents/ trees are deliberately NOT consulted when the +# root came from the plugins/ probe. 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. +# Deployed trees ARE used when no plugin monorepo was found — whether the walk +# landed on a bare .git ancestor or on nothing at all. That is the consumer +# case: the file being checked lives in or beside a deployed tree, inside an +# ordinary git repo, with no monorepo to read. The two cases are told apart by +# which probe matched, never by how many names a root contributed; see +# known_targets(). def _is_fs_root(path): @@ -241,11 +245,17 @@ def _is_fs_root(path): def _collect_package(pkg_dir, names): """Add every skill/agent name a package directory exposes, any layout.""" + # glob.escape() the DIRECTORY only. A checkout path containing `[`, `]`, + # `*` or `?` — a worktree named `feature[2]`, say — otherwise turns the + # whole pattern into a character class that matches nothing, and the + # resolver degrades to the "DID NOT RUN" INFO with rc=0 across every file + # in the tree. The wildcards in `sub` are the intended ones and stay raw. + safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): - for path in glob.glob(os.path.join(pkg_dir, sub)): + for path in glob.glob(os.path.join(safe_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)): + for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) if base.endswith('.agent.md'): base = base[:-len('.agent.md')] @@ -277,28 +287,35 @@ def _apm_package_root(start_dir): def _authoring_root(start_dir): """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + Returns (root, matched_plugins_probe). The flag reports WHICH probe + matched: True for the plugins/*/.apm/{skills,agents} glob, False for the + .git fallback and for no match at all. known_targets() needs that + distinction — only a real plugins/ root makes the deployed trees + redundant, and a name-count delta cannot tell the two apart. + 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'))): + probes = ( + lambda d: bool(glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))) + for index, probe in enumerate(probes): current = os.path.abspath(start_dir) for _ in range(12): if _is_fs_root(current): break if probe(current): - return current + return current, index == 0 current = os.path.dirname(current) - return None + return None, False def _collect_authoring_root(root, names): """Every plugin in the monorepo contributes its names.""" - for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + for pkg in glob.glob(os.path.join(glob.escape(root), 'plugins', '*')): if os.path.isdir(pkg): _collect_package(pkg, names) @@ -362,7 +379,7 @@ def _declared_dependency_dirs(pkg_dir): 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 + Consulted ONLY when no plugin monorepo root was found; see the 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. """ @@ -401,10 +418,22 @@ def known_targets(start_dir): for dep_dir in _declared_dependency_dirs(package): _collect_package(dep_dir, names) - root = _authoring_root(start) + # A .git ancestor is an authoring root only if it actually holds plugins. + # _authoring_root() falls back to the nearest .git, so it is truthy in ANY + # git repo; without the distinction that fallback wins in every consumer + # checkout, _collect_authoring_root() contributes nothing, and the deployed + # branch below is dead code in the exact case it exists for. So condition + # on WHICH probe matched, which _authoring_root() reports directly. A + # name-count delta looks equivalent and is not: _collect_authoring_root() + # re-collects the checked file's own plugin, whose names the blocks above + # already added, so a one-plugin monorepo shows a delta of zero and would + # wrongly reach for the deployed trees — including the user's global + # ~/.claude/skills, making the verdict depend on what happens to be + # installed (ADR-0020 lines 118-127). + root, root_has_plugins = _authoring_root(start) if root: _collect_authoring_root(root, names) - else: + if not root_has_plugins: for base in _deployed_roots(start): _collect_package(base, names) return names @@ -510,11 +539,17 @@ MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +# re.I on ALL of them, uniformly. The patterns are built from the same +# lowercase NAME_* fragments, so half of them carrying the flag and half not +# meant `Skill-Audit` at the start of a boundary sentence was extracted by +# ROUTE_ANY but invisible to BACKTICK — contradicting normalize_target()'s own +# docstring, which exists precisely because extraction is case-insensitive and +# the universe is not. +CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) +CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) +ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # 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) @@ -681,8 +716,16 @@ def unresolved_targets(description, known): # 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. +# +# The CLOSING marker is anchored at column 0 — deliberately NOT `[ \t]*---`. +# YAML block-scalar content must be indented deeper than its key, so an +# indented `---` inside a folded description is CONTENT; letting it close the +# frontmatter truncated the description mid-value and silently reclassified the +# rest as body, which is a vacuous green in both directions at once. Leading +# whitespace is still tolerated on the OPENING marker, where no such content +# can exist. FRONTMATTER_RE = re.compile( - r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)', re.DOTALL) def strip_bom(text): @@ -714,14 +757,26 @@ def description_value(fm_text): try: data = yaml.safe_load(fm_text) except Exception as exc: - raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + # Every FrontmatterError message is a COMPLETE clause, never a detail a + # caller wraps in one. Callers used to prefix a hard-coded "frontmatter + # is not valid YAML (...)", which is true only of this branch: the two + # type failures below come from frontmatter that parsed fine, and + # telling their author the YAML is invalid sends them hunting for a + # syntax error that is not there — on a blocking gate with no baseline. + raise FrontmatterError('frontmatter is not valid YAML (%s)' + % 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) + # NOT str()-coerced. `description: true` became the 4-character "True" + # and sailed through the 400-character gate; a list or mapping was + # measured as its Python repr. Neither is a description a host can + # preload, so this is a parse failure, reported as one. + raise FrontmatterError( + 'description is a %s, not a string' % type(value).__name__) return re.sub(r'\s+', ' ', value).strip() @@ -791,6 +846,18 @@ def mask_fenced(text): if (marker and marker[0] == fence[0] and len(marker) >= len(fence) and not stripped.strip()[len(marker):].strip()): fence = None + # An UNCLOSED fence has no cost-free answer, only a choice of which way to + # be wrong. Masking to end-of-body blanks the rest of the body, silently + # disabling the ERROR-tier references/ check and the gotcha counts. + # Returning the raw text instead exposes the unclosed example's own + # content, so a fenced example naming a nonexistent references/ file + # becomes a hard ERROR it would not have been had the fence been closed — + # confirmed, not hypothetical. The loud-false-positive direction is the one + # chosen: this script's rule is that a file it cannot measure must never + # report green, and masking-onward is exactly that failure. Both outcomes + # need an already-malformed file, and the false positive costs one fence. + if fence is not None: + return text return ''.join(out) @@ -869,12 +936,15 @@ def get_frontmatter_keys(fm): return keys def agent_description(fm, local_fname): - """The folded description VALUE, or None if the frontmatter is not YAML.""" + """The folded description VALUE, or None if it could not be read.""" try: 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}") + # `exc` carries the whole clause — invalid YAML, a non-mapping block, or + # a description of the wrong type. Do not prefix a diagnosis here; the + # last one named a syntax error for two failures that have none. + fail(f"{exc} — the ADR-0020 description and boundary-target gates could " + f"not run — {local_fname}") return None def check_description_budget(value, local_fname): @@ -944,11 +1014,31 @@ def check_boundary(value, fpath, local_fname): 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') - if not val: + """Tool names from the `tools` field — inline scalar OR YAML block sequence. + + Read off the PARSED mapping, never off extract_field(). That function's + capture is newline-bounded on purpose (`[^\\S\\r\\n]*(.+)`), so a `tools:` + written as a block sequence — the shape Copilot agent files use — captured + nothing at all and the subagent-unavailable-tool check silently stopped + firing on exactly the files it was written for. Both spellings are legal + YAML, so both are read here. + """ + try: + data = yaml.safe_load(fm) + except Exception: + # Not this function's failure to report: the frontmatter's validity is + # decided (and failed) by agent_description() on the same text. return set() - return set(re.split(r'[\s,]+', val.strip())) + if not isinstance(data, dict): + return set() + val = data.get('tools') + if isinstance(val, list): + items = [str(item).strip() for item in val] + elif isinstance(val, str): + items = re.split(r'[\s,]+', val.strip()) + else: + return set() + return {item for item in items if item} def is_copilot_cloud_ide(fpath): """True if the file is a cloud/IDE Copilot agent (name is optional for these).""" @@ -1063,6 +1153,15 @@ def check_apm_agent_file(fpath, allowlist, stem): fail(f"file is {exc}. Nothing could be measured, so this is a hard " f"failure, not a skip — {local_fname}") return + except OSError as exc: + # A path that cannot be opened gets a FAIL line naming it, not a bare + # FileNotFoundError traceback. scripts/check-apm-agents-valid.sh takes + # this path for an agent file deleted from the worktree but still + # tracked in the index — a real, expected state, and the caller needs to + # be told which file, not handed an interpreter stack. + fail(f"could not be read ({exc.strerror or exc}): {fpath}. Nothing could " + f"be measured, so this is a hard failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: @@ -1168,6 +1267,13 @@ def check_file(fpath, file_provider): fail(f"file is {exc}. Nothing could be measured, so this is a hard " f"failure, not a skip — {local_fname}") return + except OSError as exc: + # Same reason as check_apm_agent_file's: a diagnostic naming the path + # beats a FileNotFoundError traceback. The counterpart is pre-checked at + # the bottom of this script, but agent_file itself never was. + fail(f"could not be read ({exc.strerror or exc}): {fpath}. Nothing could " + f"be measured, so this is a hard failure, not a skip — {local_fname}") + return fm, body = parse_frontmatter(content) if fm is None: diff --git a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh index 35c9733..f80f02f 100755 --- a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh @@ -148,17 +148,21 @@ def read_text(path): # 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 .claude/ and .agents/ trees are deliberately NOT consulted when the +# root came from the plugins/ probe. 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. +# Deployed trees ARE used when no plugin monorepo was found — whether the walk +# landed on a bare .git ancestor or on nothing at all. That is the consumer +# case: the file being checked lives in or beside a deployed tree, inside an +# ordinary git repo, with no monorepo to read. The two cases are told apart by +# which probe matched, never by how many names a root contributed; see +# known_targets(). def _is_fs_root(path): @@ -167,11 +171,17 @@ def _is_fs_root(path): def _collect_package(pkg_dir, names): """Add every skill/agent name a package directory exposes, any layout.""" + # glob.escape() the DIRECTORY only. A checkout path containing `[`, `]`, + # `*` or `?` — a worktree named `feature[2]`, say — otherwise turns the + # whole pattern into a character class that matches nothing, and the + # resolver degrades to the "DID NOT RUN" INFO with rc=0 across every file + # in the tree. The wildcards in `sub` are the intended ones and stay raw. + safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): - for path in glob.glob(os.path.join(pkg_dir, sub)): + for path in glob.glob(os.path.join(safe_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)): + for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) if base.endswith('.agent.md'): base = base[:-len('.agent.md')] @@ -203,28 +213,35 @@ def _apm_package_root(start_dir): def _authoring_root(start_dir): """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + Returns (root, matched_plugins_probe). The flag reports WHICH probe + matched: True for the plugins/*/.apm/{skills,agents} glob, False for the + .git fallback and for no match at all. known_targets() needs that + distinction — only a real plugins/ root makes the deployed trees + redundant, and a name-count delta cannot tell the two apart. + 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'))): + probes = ( + lambda d: bool(glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))) + for index, probe in enumerate(probes): current = os.path.abspath(start_dir) for _ in range(12): if _is_fs_root(current): break if probe(current): - return current + return current, index == 0 current = os.path.dirname(current) - return None + return None, False def _collect_authoring_root(root, names): """Every plugin in the monorepo contributes its names.""" - for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + for pkg in glob.glob(os.path.join(glob.escape(root), 'plugins', '*')): if os.path.isdir(pkg): _collect_package(pkg, names) @@ -288,7 +305,7 @@ def _declared_dependency_dirs(pkg_dir): 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 + Consulted ONLY when no plugin monorepo root was found; see the 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. """ @@ -327,10 +344,22 @@ def known_targets(start_dir): for dep_dir in _declared_dependency_dirs(package): _collect_package(dep_dir, names) - root = _authoring_root(start) + # A .git ancestor is an authoring root only if it actually holds plugins. + # _authoring_root() falls back to the nearest .git, so it is truthy in ANY + # git repo; without the distinction that fallback wins in every consumer + # checkout, _collect_authoring_root() contributes nothing, and the deployed + # branch below is dead code in the exact case it exists for. So condition + # on WHICH probe matched, which _authoring_root() reports directly. A + # name-count delta looks equivalent and is not: _collect_authoring_root() + # re-collects the checked file's own plugin, whose names the blocks above + # already added, so a one-plugin monorepo shows a delta of zero and would + # wrongly reach for the deployed trees — including the user's global + # ~/.claude/skills, making the verdict depend on what happens to be + # installed (ADR-0020 lines 118-127). + root, root_has_plugins = _authoring_root(start) if root: _collect_authoring_root(root, names) - else: + if not root_has_plugins: for base in _deployed_roots(start): _collect_package(base, names) return names @@ -436,11 +465,17 @@ MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +# re.I on ALL of them, uniformly. The patterns are built from the same +# lowercase NAME_* fragments, so half of them carrying the flag and half not +# meant `Skill-Audit` at the start of a boundary sentence was extracted by +# ROUTE_ANY but invisible to BACKTICK — contradicting normalize_target()'s own +# docstring, which exists precisely because extraction is case-insensitive and +# the universe is not. +CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) +CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) +ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # 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) @@ -607,8 +642,16 @@ def unresolved_targets(description, known): # 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. +# +# The CLOSING marker is anchored at column 0 — deliberately NOT `[ \t]*---`. +# YAML block-scalar content must be indented deeper than its key, so an +# indented `---` inside a folded description is CONTENT; letting it close the +# frontmatter truncated the description mid-value and silently reclassified the +# rest as body, which is a vacuous green in both directions at once. Leading +# whitespace is still tolerated on the OPENING marker, where no such content +# can exist. FRONTMATTER_RE = re.compile( - r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)', re.DOTALL) def strip_bom(text): @@ -640,14 +683,26 @@ def description_value(fm_text): try: data = yaml.safe_load(fm_text) except Exception as exc: - raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + # Every FrontmatterError message is a COMPLETE clause, never a detail a + # caller wraps in one. Callers used to prefix a hard-coded "frontmatter + # is not valid YAML (...)", which is true only of this branch: the two + # type failures below come from frontmatter that parsed fine, and + # telling their author the YAML is invalid sends them hunting for a + # syntax error that is not there — on a blocking gate with no baseline. + raise FrontmatterError('frontmatter is not valid YAML (%s)' + % 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) + # NOT str()-coerced. `description: true` became the 4-character "True" + # and sailed through the 400-character gate; a list or mapping was + # measured as its Python repr. Neither is a description a host can + # preload, so this is a parse failure, reported as one. + raise FrontmatterError( + 'description is a %s, not a string' % type(value).__name__) return re.sub(r'\s+', ' ', value).strip() @@ -717,6 +772,18 @@ def mask_fenced(text): if (marker and marker[0] == fence[0] and len(marker) >= len(fence) and not stripped.strip()[len(marker):].strip()): fence = None + # An UNCLOSED fence has no cost-free answer, only a choice of which way to + # be wrong. Masking to end-of-body blanks the rest of the body, silently + # disabling the ERROR-tier references/ check and the gotcha counts. + # Returning the raw text instead exposes the unclosed example's own + # content, so a fenced example naming a nonexistent references/ file + # becomes a hard ERROR it would not have been had the fence been closed — + # confirmed, not hypothetical. The loud-false-positive direction is the one + # chosen: this script's rule is that a file it cannot measure must never + # report green, and masking-onward is exactly that failure. Both outcomes + # need an already-malformed file, and the false positive costs one fence. + if fence is not None: + return text return ''.join(out) @@ -802,8 +869,11 @@ name = name_m.group(1).strip('"\'') if name_m else "" 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") + # `exc` carries the whole clause — invalid YAML, a non-mapping block, or a + # description of the wrong type. Do not prefix a diagnosis here; the last + # one named a syntax error for two failures that have none. + fail(f"{exc}. Nothing downstream can be measured, so this is a hard " + f"failure, not a skip") print("One or more checks failed.") sys.exit(1) diff --git a/scripts/skill-size-check.sh b/scripts/skill-size-check.sh index 906535b..5a27b56 100755 --- a/scripts/skill-size-check.sh +++ b/scripts/skill-size-check.sh @@ -30,8 +30,11 @@ set -euo pipefail # SKILL.md could pass its own audit and still be blocked by the commit hook. # The ADR-0020 ceilings are inclusive the same way. # -# Token counts aren't computed exactly here — word count (`wc -w`) is used as -# a proxy. Measured over this repo's 39 in-scope SKILL.md files, characters per +# Token counts aren't computed exactly here — a whitespace word count is used +# as a proxy (Python's str.split(), the same primitive +# skill-audit/scripts/validate.sh applies to these two constants; `wc -w` +# disagrees with it on Unicode separators, which is why the awk pass that used +# to live in the loop below is gone). Measured over this repo's 39 in-scope SKILL.md files, characters per # word runs min 5.97 / median 6.79 / mean 6.77 / max 7.22. At the standard # ~4-characters-per-token English approximation that is 1.49 / 1.70 / 1.69 / # 1.81 tokens per word. @@ -105,23 +108,19 @@ for f in "$@"; do 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 - # trailing newline, matching Python's splitlines() semantics (used by - # skill-audit/scripts/validate.sh for its own line count) — `wc -l` - # undercounts by 1 in that case. Word count uses awk's default - # whitespace-splitting NF, matching `wc -w` semantics. - read -r lines words <<< "$(awk '{w += NF} END{print NR, w+0}' "$f")" - - if (( lines > MAX_LINES )); then - echo "ERROR: $f has $lines lines, exceeding the $MAX_LINES-line ceiling (agentskills.io skill-authoring.md)" >&2 - FAIL=1 - fi - - if (( words > MAX_WORDS )); then - echo "ERROR: $f has $words words (proxy for tokens), exceeding the $MAX_WORDS-word ceiling (~5,000 tokens, agentskills.io skill-authoring.md)" >&2 - FAIL=1 - fi + # The MAX_LINES / MAX_WORDS ceilings are NOT measured here. They used to be, + # in a single awk pass, and that pass was wrong twice over: + # * `read -r lines words <<< "$(awk ...)"` discarded awk's exit status, so a + # file awk could not read yielded empty variables, bash arithmetic read + # them as 0, and both ceilings passed in total silence — the one outcome + # this script forbids itself. + # * awk's NR/NF do not agree with the Python splitlines()/split() that + # skill-audit/scripts/validate.sh uses for the SAME two constants. + # splitlines() also breaks on \x0b \x0c \x1c \x1d \x1e \x85 U+2028 U+2029 + # and split() on every Unicode space, so a body padded with U+2028 read as + # 6 lines here and 606 lines there — hook green, audit FAIL. + # One implementation now owns both: the Python block below already reads every + # file (with a real diagnostic on failure), so it counts there. done if ! command -v python3 > /dev/null 2>&1; then @@ -140,7 +139,7 @@ fi if ! python3 -u - \ "$DESC_SUGGEST_CHARS" "$DESC_MAX_CHARS" \ - "$BODY_SUGGEST_WORDS" "$BODY_MAX_WORDS" "$MAX_WORDS" "$@" <<'PYTHON' + "$BODY_SUGGEST_WORDS" "$BODY_MAX_WORDS" "$MAX_WORDS" "$MAX_LINES" "$@" <<'PYTHON' import glob import os import re @@ -153,7 +152,8 @@ 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:] +MAX_LINES = int(sys.argv[6]) +files = sys.argv[7:] failed = False @@ -232,17 +232,21 @@ def read_text(path): # 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 .claude/ and .agents/ trees are deliberately NOT consulted when the +# root came from the plugins/ probe. 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. +# Deployed trees ARE used when no plugin monorepo was found — whether the walk +# landed on a bare .git ancestor or on nothing at all. That is the consumer +# case: the file being checked lives in or beside a deployed tree, inside an +# ordinary git repo, with no monorepo to read. The two cases are told apart by +# which probe matched, never by how many names a root contributed; see +# known_targets(). def _is_fs_root(path): @@ -251,11 +255,17 @@ def _is_fs_root(path): def _collect_package(pkg_dir, names): """Add every skill/agent name a package directory exposes, any layout.""" + # glob.escape() the DIRECTORY only. A checkout path containing `[`, `]`, + # `*` or `?` — a worktree named `feature[2]`, say — otherwise turns the + # whole pattern into a character class that matches nothing, and the + # resolver degrades to the "DID NOT RUN" INFO with rc=0 across every file + # in the tree. The wildcards in `sub` are the intended ones and stay raw. + safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): - for path in glob.glob(os.path.join(pkg_dir, sub)): + for path in glob.glob(os.path.join(safe_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)): + for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) if base.endswith('.agent.md'): base = base[:-len('.agent.md')] @@ -287,28 +297,35 @@ def _apm_package_root(start_dir): def _authoring_root(start_dir): """Nearest ancestor that is a plugin monorepo, else the nearest .git tree. + Returns (root, matched_plugins_probe). The flag reports WHICH probe + matched: True for the plugins/*/.apm/{skills,agents} glob, False for the + .git fallback and for no match at all. known_targets() needs that + distinction — only a real plugins/ root makes the deployed trees + redundant, and a name-count delta cannot tell the two apart. + 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'))): + probes = ( + lambda d: bool(glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'skills')) + or glob.glob(os.path.join(glob.escape(d), 'plugins', '*', '.apm', 'agents'))), + lambda d: os.path.exists(os.path.join(d, '.git'))) + for index, probe in enumerate(probes): current = os.path.abspath(start_dir) for _ in range(12): if _is_fs_root(current): break if probe(current): - return current + return current, index == 0 current = os.path.dirname(current) - return None + return None, False def _collect_authoring_root(root, names): """Every plugin in the monorepo contributes its names.""" - for pkg in glob.glob(os.path.join(root, 'plugins', '*')): + for pkg in glob.glob(os.path.join(glob.escape(root), 'plugins', '*')): if os.path.isdir(pkg): _collect_package(pkg, names) @@ -372,7 +389,7 @@ def _declared_dependency_dirs(pkg_dir): 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 + Consulted ONLY when no plugin monorepo root was found; see the 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. """ @@ -411,10 +428,22 @@ def known_targets(start_dir): for dep_dir in _declared_dependency_dirs(package): _collect_package(dep_dir, names) - root = _authoring_root(start) + # A .git ancestor is an authoring root only if it actually holds plugins. + # _authoring_root() falls back to the nearest .git, so it is truthy in ANY + # git repo; without the distinction that fallback wins in every consumer + # checkout, _collect_authoring_root() contributes nothing, and the deployed + # branch below is dead code in the exact case it exists for. So condition + # on WHICH probe matched, which _authoring_root() reports directly. A + # name-count delta looks equivalent and is not: _collect_authoring_root() + # re-collects the checked file's own plugin, whose names the blocks above + # already added, so a one-plugin monorepo shows a delta of zero and would + # wrongly reach for the deployed trees — including the user's global + # ~/.claude/skills, making the verdict depend on what happens to be + # installed (ADR-0020 lines 118-127). + root, root_has_plugins = _authoring_root(start) if root: _collect_authoring_root(root, names) - else: + if not root_has_plugins: for base in _deployed_roots(start): _collect_package(base, names) return names @@ -520,11 +549,17 @@ MARKED_TARGET = r"(?:`/?(%s)`|(?|→)\s*%s" % MARKED_TARGET) +# re.I on ALL of them, uniformly. The patterns are built from the same +# lowercase NAME_* fragments, so half of them carrying the flag and half not +# meant `Skill-Audit` at the start of a boundary sentence was extracted by +# ROUTE_ANY but invisible to BACKTICK — contradicting normalize_target()'s own +# docstring, which exists precisely because extraction is case-insensitive and +# the universe is not. +CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) +CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) +ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) -BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH) +BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # 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) @@ -691,8 +726,16 @@ def unresolved_targets(description, known): # 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. +# +# The CLOSING marker is anchored at column 0 — deliberately NOT `[ \t]*---`. +# YAML block-scalar content must be indented deeper than its key, so an +# indented `---` inside a folded description is CONTENT; letting it close the +# frontmatter truncated the description mid-value and silently reclassified the +# rest as body, which is a vacuous green in both directions at once. Leading +# whitespace is still tolerated on the OPENING marker, where no such content +# can exist. FRONTMATTER_RE = re.compile( - r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n[ \t]*---[ \t]*(?:\r?\n|\Z)', re.DOTALL) + r'^[ \t\r\n]*---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)', re.DOTALL) def strip_bom(text): @@ -724,14 +767,26 @@ def description_value(fm_text): try: data = yaml.safe_load(fm_text) except Exception as exc: - raise FrontmatterError(re.sub(r'\s+', ' ', str(exc)).strip()) + # Every FrontmatterError message is a COMPLETE clause, never a detail a + # caller wraps in one. Callers used to prefix a hard-coded "frontmatter + # is not valid YAML (...)", which is true only of this branch: the two + # type failures below come from frontmatter that parsed fine, and + # telling their author the YAML is invalid sends them hunting for a + # syntax error that is not there — on a blocking gate with no baseline. + raise FrontmatterError('frontmatter is not valid YAML (%s)' + % 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) + # NOT str()-coerced. `description: true` became the 4-character "True" + # and sailed through the 400-character gate; a list or mapping was + # measured as its Python repr. Neither is a description a host can + # preload, so this is a parse failure, reported as one. + raise FrontmatterError( + 'description is a %s, not a string' % type(value).__name__) return re.sub(r'\s+', ' ', value).strip() @@ -801,6 +856,18 @@ def mask_fenced(text): if (marker and marker[0] == fence[0] and len(marker) >= len(fence) and not stripped.strip()[len(marker):].strip()): fence = None + # An UNCLOSED fence has no cost-free answer, only a choice of which way to + # be wrong. Masking to end-of-body blanks the rest of the body, silently + # disabling the ERROR-tier references/ check and the gotcha counts. + # Returning the raw text instead exposes the unclosed example's own + # content, so a fenced example naming a nonexistent references/ file + # becomes a hard ERROR it would not have been had the fence been closed — + # confirmed, not hypothetical. The loud-false-positive direction is the one + # chosen: this script's rule is that a file it cannot measure must never + # report green, and masking-onward is exactly that failure. Both outcomes + # need an already-malformed file, and the false positive costs one fence. + if fence is not None: + return text return ''.join(out) @@ -868,12 +935,28 @@ for path in files: "silence." % (path, why)) continue try: - content = strip_bom(read_text(path)) + raw = read_text(path) except EncodingError as exc: - error("%s: %s. None of the ADR-0020 gates could run on this file." - % (path, exc)) + error("%s: %s. Neither the spec line/word ceilings nor any of the " + "ADR-0020 gates could run on this file." % (path, exc)) continue + # SPEC CONFORMANCE (family 1). Whole file, frontmatter included, counted + # with the SAME primitives skill-audit/scripts/validate.sh uses for these + # two constants — see the note in the bash loop above for what the previous + # awk pass got wrong. + lines = len(raw.splitlines()) + words = len(raw.split()) + if lines > MAX_LINES: + error("%s has %d lines, exceeding the %d-line ceiling " + "(agentskills.io skill-authoring.md)" % (path, lines, MAX_LINES)) + if words > MAX_WORDS: + error("%s has %d words (proxy for tokens), exceeding the %d-word ceiling " + "(~5,000 tokens, agentskills.io skill-authoring.md)" + % (path, words, MAX_WORDS)) + + content = strip_bom(raw) + fm_match = FRONTMATTER_RE.match(content) if not fm_match: error("%s: no parseable YAML frontmatter block. Expected a `---` line, " @@ -887,8 +970,11 @@ for path in files: 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)) + # `exc` carries the whole clause — invalid YAML, a non-mapping block, or + # a description of the wrong type. Do not prefix a diagnosis here; the + # last one named a syntax error for two failures that have none. + error("%s: %s. None of the ADR-0020 gates could run on this file." + % (path, exc)) continue body = content[fm_match.end():]