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():]