diff --git a/plugins/skill-quality/.claude-plugin/plugin.json b/plugins/skill-quality/.claude-plugin/plugin.json index 6b02f4229..f00d51bb7 100644 --- a/plugins/skill-quality/.claude-plugin/plugin.json +++ b/plugins/skill-quality/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "skill-quality", - "version": "0.15.2", + "version": "0.15.3", "description": "Skill-authoring QA tooling: a static contract checker that runs twenty-two deterministic checks over a Claude Code skill (frontmatter, per-skill listing-entry cap, trigger-keyword preservation, line caps, broken internal refs, markdownlint, gotchas surface, evals presence, precompute opportunity, injection shell-declaration, fresh-eyes declaration conformance), a shared skill-listing budget reporter across a set of skills, and a bundled evals.json schema plus a deterministic eval-quality lint (duplicate case identities, missing fixtures, empty or vague grading criteria, set-coverage warnings). Runs against any repo's skills directory via the convention-resolution ladder — no baked layout.", "author": { "name": "Melodic Software", diff --git a/plugins/skill-quality/CHANGELOG.md b/plugins/skill-quality/CHANGELOG.md index d51764fe4..0a6345b87 100644 --- a/plugins/skill-quality/CHANGELOG.md +++ b/plugins/skill-quality/CHANGELOG.md @@ -3,6 +3,52 @@ All notable changes to the `skill-quality` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.15.3] + +### Fixed + +- **`check-listing-budget.sh` measures in ONE `awk` pass, so the command the quarterly routine tells + an operator to run can actually finish (#2216).** `bash check-listing-budget.sh plugins/*/skills` + took **289s** on Windows (Git Bash) and, run in the foreground by an agent, was **killed at 180s + with exit 143 and zero output**. Re-measured at this branch's merge base before the port: **232s** + for the same command. The cause was process-spawn cost, not the machine — the per-file loop spent + at least **eleven forked subshells and five external process execs** (4x `awk`, 1x `tr`) on every + one of the repo's ~200 `SKILL.md` files, on the order of 2,000 spawns. Process creation costs + roughly two orders of magnitude more on Windows than on Linux, which is why CI (`ubuntu-24.04`) + never surfaced it. That command is verbatim what #2023's procedure and + `.github/recurring-schedule.json`'s `listing-budget-watch` row instruct, so on the one machine + where the routine is actually driven, a report-only drift watch silently produced nothing. + + The same run now takes **2.2s** — and the output is **byte-identical**, which is the point rather + than a hope. Both implementations were run over the same tree and their per-file contribution rows + diffed, not just their reports: **144/144 rows identical**, aggregate 94,468 identical, top-10 + ordering identical. Per-file rows are the load-bearing comparison — two files with offsetting + extraction errors produce a matching aggregate and a clean report diff while the parser is broken. + + This is a **port, not a rewrite**: the awk program reimplements, behaviour for behaviour, the four + helpers the loop shelled out to — `skill_frontmatter::extract`, `::field` (block-scalar unfolding + for `|` and `>`, and the quote-aware trailing-comment strip including the doubled-single-quote + case), `::strip_quotes` (one outer layer, double OR single, never both), and + `normalize_bool`/`trim_ws`. Two behaviours the old pipeline got free from command substitution are + reproduced explicitly and commented as such: trailing newlines stripped from the extracted + frontmatter (so a trailing blank line cannot add a separator inside a block scalar, and an + all-blank block counts as no frontmatter), and trailing newlines stripped from each field's value. + Because corpus equivalence is not parser equivalence, four new fixtures pin the shapes most likely + to diverge: a literal `|` block with a trailing blank, a single-quoted scalar with a doubled quote + plus a trailing comment, and a multibyte description asserted **as an equivalence against the + shell's own `${#var}`** rather than a hardcoded count — awk `length()` and `${#var}` can disagree + where awk counts bytes and the shell counts characters, and a fixed number would encode one + environment's answer and fail elsewhere for the wrong reason. + + A fifth case bounds the wall clock over a 200-file corpus at **30s**. The bound is deliberately + very loose against a ~1s target: this runs in required CI, and a tight timing assertion is a flaky + gate — worse than the defect it guards. It fails only on a return to per-file process spawning, + which is two orders of magnitude away. + + `skill-frontmatter.sh` is no longer sourced here (its helpers are the per-call execs that caused + this); the library file is unchanged and `check-skill.sh` remains its consumer. No flag, no + option, and no output format changed. + ## [0.15.2] ### Changed diff --git a/plugins/skill-quality/scripts/check-listing-budget.sh b/plugins/skill-quality/scripts/check-listing-budget.sh index 6e229a377..34089ae2c 100755 --- a/plugins/skill-quality/scripts/check-listing-budget.sh +++ b/plugins/skill-quality/scripts/check-listing-budget.sh @@ -109,8 +109,6 @@ # in the issue this script closes. set -uo pipefail -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - # The header comment block above IS the --help text. Print from line 2 to the # last consecutive `#` line rather than a hardcoded range, so editing the # header can never again silently clip or overrun the help output. @@ -129,30 +127,11 @@ if [[ -z "$REPO_ROOT" || ! -d "$REPO_ROOT" ]]; then exit 2 fi -# shellcheck source=./skill-frontmatter.sh -source "$SCRIPT_DIR/skill-frontmatter.sh" - -# Fold a frontmatter boolean to a bare lowercase token before comparing it. -# `skill_frontmatter::field` returns the scalar with any trailing YAML comment -# already cut, so what remains to fold is surrounding whitespace and quoted or -# differently-cased forms — an exact-string compare against "true" misses those -# and silently re-counts a skill whose description the harness keeps out of -# context. -# Deliberately NOT folded: YAML 1.1's `yes` / `on` aliases — the docs only ever -# spell this field `true`, and treating a bare `yes` as the boolean risks -# dropping a skill over a value the harness may read as a plain string. -# A pragmatic normalizer for one known field, not a YAML parser. -trim_ws() { - local v="$1" - v="${v#"${v%%[![:space:]]*}"}" - printf '%s' "${v%"${v##*[![:space:]]}"}" -} - -normalize_bool() { - local v - v="$(trim_ws "$(skill_frontmatter::strip_quotes "$(trim_ws "$1")")")" - printf '%s' "$(tr '[:upper:]' '[:lower:]' <<<"$v")" -} +# `skill-frontmatter.sh` is deliberately NOT sourced any more. Its helpers are +# per-call `awk`/`tr` execs, which is what made the pooled run unrunnable (see +# the measurement scan below); their behaviour is ported into that scan's single +# awk program instead. `check-skill.sh` remains the library's consumer, so the +# file itself is unchanged and still shared. # Reject a nonnumeric override up front. Without this, `awk` coerces a typo to # 0 and the report exits 0 announcing a zero-character budget and a bogus @@ -240,12 +219,14 @@ fi # --- Collect every skill under every root ----------------------------------- -TOTAL=0 -ENTRY_COUNT=0 CONTRIB_FILE="$(mktemp)" -trap 'rm -f "$CONTRIB_FILE"' EXIT +FILE_LIST="$(mktemp)" +trap 'rm -f "$CONTRIB_FILE" "$FILE_LIST"' EXIT +# Enumerate first, measure once. The bash half below only globs and stats — no +# subshell, no exec — and the whole measurement is ONE awk process. FOUND_ROOTS=0 +SKILL_ENTRIES=() for root in "${ROOTS[@]}"; do if [[ ! -d "$root" ]]; then # An EXPLICIT root that does not exist is an environment error: silently @@ -261,32 +242,184 @@ for root in "${ROOTS[@]}"; do FOUND_ROOTS=$((FOUND_ROOTS + 1)) for skill_md in "$root"/*/SKILL.md; do [[ -f "$skill_md" ]] || continue - skill_name="${skill_md%/SKILL.md}" - skill_name="${skill_name##*/}" - fm="$(skill_frontmatter::extract <"$skill_md")" - [[ -n "$fm" ]] || continue - # `disable-model-invocation: true` keeps this skill's description out of - # the model-visible listing entirely, so it spends none of the shared - # budget — counting it would overstate the aggregate. See the header. - dmi="$(normalize_bool "$(skill_frontmatter::field disable-model-invocation <<<"$fm")")" - [[ "$dmi" == "true" ]] && continue - desc="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field description <<<"$fm")")" - wtu="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field when_to_use <<<"$fm")")" - desc_len=${#desc} - wtu_len=${#wtu} - joiner=0 - ((wtu_len > 0)) && joiner=$JOINER_CHARS - entry_len=$((desc_len + joiner + wtu_len)) + # Root and path travel together rather than the root being re-derived from + # the path: the report prints the root STRING the caller passed, and a + # derivation would have to reproduce its exact spelling (trailing slash, + # relative vs absolute) to stay byte-identical. + SKILL_ENTRIES+=("$root"$'\t'"$skill_md") + done +done +if ((${#SKILL_ENTRIES[@]} > 0)); then + printf '%s\n' "${SKILL_ENTRIES[@]}" >"$FILE_LIST" +fi + +# --- One awk pass over every SKILL.md --------------------------------------- +# +# WHY THIS IS ONE PROCESS. The previous shape was a bash loop that spent at +# least eleven forked subshells and five external process execs (4x awk, 1x tr) +# on EVERY SKILL.md — on the order of 2,000 spawns for this repo's ~200 skills. +# Process creation costs roughly two orders of magnitude more on Windows than on +# Linux, so `check-listing-budget.sh plugins/*/skills` took 289s there and was +# KILLED at the 180s foreground limit with zero output, while CI (ubuntu-24.04) +# never surfaced it. That command is verbatim what #2023's procedure and +# .github/recurring-schedule.json instruct an operator to run each cycle, so a +# report-only drift watch silently produced nothing on the one machine where the +# routine is actually driven (#2216). +# +# THIS IS A PORT, NOT A REWRITE. The program below reimplements, behaviour for +# behaviour, the four helpers the loop used to shell out to: +# `skill_frontmatter::extract`, `::field` (including block-scalar unfolding for +# `|` and `>`, and the quote-aware trailing-comment strip with its +# doubled-single-quote case), `::strip_quotes` (ONE outer layer, double OR +# single, never both), and `normalize_bool`/`trim_ws`. Output is byte-identical +# to the pre-port script over this repo's tree — aggregate, entry count, every +# per-file contribution row, and the report text — which is the property the +# test suite pins. +# +# Two command-substitution behaviours the old pipeline got for free are +# reproduced EXPLICITLY here, because they were load-bearing rather than +# incidental: +# - `fm="$(skill_frontmatter::extract ...)"` stripped trailing NEWLINES from +# the extracted frontmatter, so trailing blank lines could never append a +# separator inside a block scalar. Hence the trailing-empty-line drop after +# collection, and hence an all-blank block counting as no frontmatter. +# - `"$(skill_frontmatter::field ...)"` stripped trailing newlines from the +# FIELD's value. Hence strip_trailing_nl() on each result. A folded (`>`) +# scalar joins with SPACES, which command substitution does not strip, so +# only newlines are removed — not whitespace generally. +# +# The file list arrives on awk's input rather than as operands: awk treats an +# operand containing `=` as a variable assignment, so a path with `=` in it +# would be silently swallowed instead of read. +# +# `disable-model-invocation: true` keeps a skill's description out of the +# model-visible listing entirely, so it spends none of the shared budget — +# counting it would overstate the aggregate. See the header. Deliberately NOT +# folded, exactly as before: YAML 1.1's `yes` / `on` aliases, because the docs +# only ever spell this field `true` and treating a bare `yes` as the boolean +# risks dropping a skill over a value the harness may read as a plain string. +# A pragmatic normalizer for one known field, not a YAML parser. +if ! AWK_RESULT="$(awk -F'\t' \ + -v max="$MAX_DESC_CHARS" -v joiner_chars="$JOINER_CHARS" -v out="$CONTRIB_FILE" ' + function trim_ws(v) { + sub(/^[[:space:]]+/, "", v) + sub(/[[:space:]]+$/, "", v) + return v + } + # ONE outer quote layer, double OR single, not both — and never a lone quote + # character, which the shell pattern `"*"` could not match either. + function strip_quotes(s, n, q) { + n = length(s) + if (n < 2) return s + q = substr(s, 1, 1) + if ((q == "\"" || q == "\047") && substr(s, n, 1) == q) return substr(s, 2, n - 2) + return s + } + function normalize_bool(v) { return tolower(trim_ws(strip_quotes(trim_ws(v)))) } + function strip_trailing_nl(v) { sub(/\n+$/, "", v); return v } + # Cut a trailing YAML comment from a plain or flow scalar. A quoted scalar + # ends at its closing quote and anything after it is comment; a plain scalar + # ends at the first whitespace-preceded `#`. An unterminated quote is left + # whole, because malformed YAML is not for this helper to guess at. + # \047 is a single quote: the program is single-quoted by the shell, so + # spelling the character out would end it mid-expression. + function fm_strip_comment(v, n, q, i, ch) { + n = length(v) + if (n == 0) return v + q = substr(v, 1, 1) + if (q == "\"" || q == "\047") { + i = 2 + while (i <= n) { + ch = substr(v, i, 1) + if (q == "\"" && ch == "\\") { i += 2; continue } + if (ch == q) { + # A single-quoted YAML scalar escapes one quote by doubling it. + if (q == "\047" && substr(v, i + 1, 1) == "\047") { i += 2; continue } + return substr(v, 1, i) + } + i++ + } + return v + } + # A value that opens with `#` is all comment: the scalar is empty. + if (q == "#") return "" + if (match(v, /[[:space:]]+#/)) return substr(v, 1, RSTART - 1) + return v + } + # Text capture for the budget count, not a full YAML parser: a frontmatter key + # sits at column 0, so every indented line is block content and the first + # column-0 line is the next key. Collecting on that boundary ignores the + # indent indicator entirely, so an explicit indent smaller than the first + # content line cannot drop later lines. Leading indent is stripped per line + # (irrelevant to a character count). Literal (`|`) joins with newlines, + # folded (`>`) with spaces. + function fm_field(k, i, val, fold, acc, started, ln, j) { + for (i = 1; i <= FN; i++) { + if (FM[i] ~ "^" k ":[[:space:]]*") { + val = FM[i] + sub("^" k ":[[:space:]]*", "", val) + if (val ~ /^[|>]([0-9][+-]?|[+-][0-9]?)?[[:space:]]*(#.*)?$/) { + fold = (val ~ /^>/) + acc = ""; started = 0 + for (j = i + 1; j <= FN; j++) { + ln = FM[j] + if (ln ~ /^[[:space:]]*$/) { if (started) acc = acc (fold ? " " : "\n"); continue } + if (ln !~ /^[[:space:]]/) break + sub(/^[[:space:]]+/, "", ln) + if (started) acc = acc (fold ? " " : "\n") + acc = acc ln + started = 1 + } + return acc + } + return fm_strip_comment(val) + } + } + return "" + } + { + root = $1; path = $2 + # Frontmatter: the opening fence MUST be line 1 — content before it is not + # frontmatter, so a stray `---` further down cannot be mistaken for the + # block start. FM is only ever read up to FN, so stale entries from a + # previous, longer file are unreachable and need no delete. + FN = 0; nr = 0; fence = 0 + while ((getline ln < path) > 0) { + nr++ + if (nr == 1 && ln !~ /^---[[:space:]]*$/) break + if (ln ~ /^---[[:space:]]*$/) { + fence++ + if (fence == 1) continue + break + } + if (fence == 1) FM[++FN] = ln + } + close(path) + while (FN > 0 && FM[FN] == "") FN-- + if (FN == 0) next + if (normalize_bool(strip_trailing_nl(fm_field("disable-model-invocation"))) == "true") next + desc = strip_quotes(strip_trailing_nl(fm_field("description"))) + wtu = strip_quotes(strip_trailing_nl(fm_field("when_to_use"))) + desc_len = length(desc); wtu_len = length(wtu) + entry_len = desc_len + (wtu_len > 0 ? joiner_chars : 0) + wtu_len # The harness truncates each entry to the per-skill cap BEFORE the shared # budget ever sees it — mirror that here so an already-oversized single - # entry (check 2's own FAIL) does not inflate this aggregate beyond what + # entry (check 2 own FAIL) does not inflate this aggregate beyond what # Claude Code would actually load. - ((entry_len > MAX_DESC_CHARS)) && entry_len=$MAX_DESC_CHARS - TOTAL=$((TOTAL + entry_len)) - ENTRY_COUNT=$((ENTRY_COUNT + 1)) - printf '%d\t%s\t%s\n' "$entry_len" "$skill_name" "$root" >>"$CONTRIB_FILE" - done -done + if (entry_len > max) entry_len = max + total += entry_len; count++ + name = path + sub(/\/SKILL\.md$/, "", name) + sub(/^.*\//, "", name) + printf "%d\t%s\t%s\n", entry_len, name, root > out + } + END { close(out); printf "%d %d\n", total, count } + ' "$FILE_LIST")"; then + printf 'Error: the single-pass frontmatter scan failed; refusing to report a partial aggregate\n' >&2 + exit 2 +fi +TOTAL="${AWK_RESULT%% *}" +ENTRY_COUNT="${AWK_RESULT##* }" if ((FOUND_ROOTS == 0)); then printf 'Error: no skills root found among: %s\n' "${ROOTS[*]}" >&2 diff --git a/plugins/skill-quality/scripts/check-listing-budget.test.sh b/plugins/skill-quality/scripts/check-listing-budget.test.sh index 6468a5705..ca46d6f13 100755 --- a/plugins/skill-quality/scripts/check-listing-budget.test.sh +++ b/plugins/skill-quality/scripts/check-listing-budget.test.sh @@ -378,6 +378,105 @@ else fail "the block fixture should measure all 9 chars of 'abc # def': $out" fi +# ===================== single-pass port guards (#2216) ====================== +# +# The measurement is one awk pass rather than ~11 forked subshells and 5 process +# execs per SKILL.md. Corpus equivalence over the repo's ~200 files (proved by +# diffing both implementations' per-file contribution rows) is NOT parser +# equivalence, so the frontmatter shapes most likely to diverge get fixtures. + +# 16. LITERAL block scalar (`|`) joins its lines with NEWLINES, and the trailing +# blank line before the closing fence must not add one — the pre-port +# pipeline got that for free from command substitution stripping trailing +# newlines, and the port has to reproduce it deliberately. +mkdir -p "$TMP/yaml-literal-root/lit" +{ + printf -- '---\n' + printf 'name: lit\n' + printf 'description: |\n' + printf ' abc\n' + printf ' de\n' + printf '\n' + printf -- '---\n\n## Purpose\n\nFixture.\n' +} >"$TMP/yaml-literal-root/lit/SKILL.md" +out="$(run "$TMP/yaml-literal-root" 2>&1)" +if grep -q 'aggregate: 6 chars' <<<"$out"; then + pass "a literal block scalar joins with newlines and drops the trailing blank" +else + fail "the literal fixture should measure 6 chars ('abc' + LF + 'de'): $out" +fi + +# 17. A SINGLE-quoted scalar escapes one quote by doubling it, so the doubled +# pair is content and the scalar does not end there. Getting this wrong +# truncates the value at the first inner quote and undercounts silently. +mkdir -p "$TMP/yaml-sq-root/sq" +{ + printf -- '---\n' + printf 'name: sq\n' + printf "description: 'it''s ok' # note\n" + printf -- '---\n\n## Purpose\n\nFixture.\n' +} >"$TMP/yaml-sq-root/sq/SKILL.md" +out="$(run "$TMP/yaml-sq-root" 2>&1)" +if grep -q 'aggregate: 8 chars' <<<"$out"; then + pass "a doubled single quote is content; the trailing comment is still cut" +else + fail "the single-quote fixture should measure 8 chars: $out" +fi + +# 18. LENGTH SEMANTICS, asserted as an EQUIVALENCE rather than a hardcoded +# number. The pre-port script measured with bash `${#var}`; the port +# measures with awk `length()`. Both are locale-dependent and they can +# disagree when awk counts bytes while the shell counts characters (mawk +# ignores the locale; gawk does not). A fixed expected number would encode +# ONE environment's answer and fail elsewhere for the wrong reason — this +# compares the script's own output against the measurement primitive the +# OLD implementation used, in the SAME shell, so a divergence is reported +# as exactly what it is. +mkdir -p "$TMP/yaml-utf8-root/utf8" +UTF8_DESC='a—b…c' +{ + printf -- '---\n' + printf 'name: utf8\n' + printf 'description: "%s"\n' "$UTF8_DESC" + printf -- '---\n\n## Purpose\n\nFixture.\n' +} >"$TMP/yaml-utf8-root/utf8/SKILL.md" +out="$(run "$TMP/yaml-utf8-root" 2>&1)" +if grep -q "aggregate: ${#UTF8_DESC} chars" <<<"$out"; then + pass "multibyte description measures the same as the shell's own \${#var} (${#UTF8_DESC} chars)" +else + fail "awk length() and bash \${#var} disagree on a multibyte value (expected ${#UTF8_DESC}): $out" +fi + +# 19. WALL CLOCK over a ~200-file corpus — the regression this port exists to +# prevent. The pooled run took 289s on Windows and was killed at the 180s +# foreground limit with zero output; the single pass returns in low +# single-digit seconds. The bound is deliberately VERY loose (30s for 200 +# files against a sub-second target): this runs in required CI, and a tight +# timing assertion is a flaky gate, which is worse than the defect it +# guards. It fails only on a return to per-file process spawning, which is +# two orders of magnitude away. +mkdir -p "$TMP/perf-root" +i=0 +while ((i < 200)); do + mkdir -p "$TMP/perf-root/skill-$i" + { + printf -- '---\n' + printf 'name: skill-%d\n' "$i" + printf 'description: "a description long enough to be representative of a real listing entry"\n' + printf 'when_to_use: "when the fixture needs a second measured field"\n' + printf -- '---\n\n## Purpose\n\nFixture.\n' + } >"$TMP/perf-root/skill-$i/SKILL.md" + i=$((i + 1)) +done +perf_start=$(date +%s) +out="$(run "$TMP/perf-root" 2>&1)" +perf_elapsed=$(($(date +%s) - perf_start)) +if grep -q '200 listing-eligible skill' <<<"$out" && ((perf_elapsed < 30)); then + pass "200-file corpus measured in ${perf_elapsed}s (bound: 30s)" +else + fail "200-file corpus took ${perf_elapsed}s or miscounted (bound 30s): $out" +fi + if [[ $fails -ne 0 ]]; then printf '%d assertion(s) failed\n' "$fails" >&2 exit 1