fix(skill-frontmatter): check every file, scope checks to frontmatter
The hook is `entry: bash` with `args: ['-c', <script>]`. pre-commit appends filenames after the script string, so the first becomes `$0` and never enters `"$@"` — on a single-file commit, the common case, the loop body never ran and the hook reported Passed having measured nothing. ADR-0022 leans on this hook as the enforcement for a mandatory `metadata.version`, so the vacuous green was the whole gate. Implementation notes: - An arg0 placeholder absorbs `$0` so every filename lands in `"$@"`. - Checks now run against the YAML frontmatter block only, extracted with awk. The old `grep -A10 "^metadata:"` matched a `metadata:` inside a body code fence, spanned past the block into a following `source:` entry's `version:`, accepted any indentation, and missed a `version:` more than ten lines in. An unreadable frontmatter block is now an error, never a pass. - The value is asserted against three-part semver. `write-docs` carried "1.0" through the entire ADR-0022 retrofit undetected, which a presence-only check cannot catch. Impact: `tests/test-skill-frontmatter.sh` is the first test this hook has ever had. It drives the real `entry`/`args` composition read out of the config rather than a copy of the script, which is the only shape that catches the arg0 bug; against the pre-fix hook it scores 7/20. gates.md described the hook wrongly in both directions and is rewritten, with a carve-out explaining why this one stays a shell parser next to the "python3 and PyYAML are hard requirements" reasoning that argues otherwise. Refs: #127 ADR: 0022 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeH8SCbcrCAQrtymkNuhKP
This commit is contained in:
@@ -254,27 +254,79 @@ repos:
|
|||||||
entry: bash
|
entry: bash
|
||||||
language: system
|
language: system
|
||||||
files: '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$'
|
files: '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$'
|
||||||
|
# Pinned by tests/test-skill-frontmatter.sh, which drives this exact
|
||||||
|
# `bash -c <script> <arg0> <files...>` call shape rather than a copy of
|
||||||
|
# the script -- the bug below was invisible to any test that did not.
|
||||||
args:
|
args:
|
||||||
- -c
|
- -c
|
||||||
- |
|
- |
|
||||||
|
# Every check reads the FRONTMATTER only, never the whole file. A
|
||||||
|
# `metadata:` / `name:` / `description:` line inside a body code
|
||||||
|
# fence is documentation (skill-author quotes exactly such a block)
|
||||||
|
# and used to satisfy these greps.
|
||||||
for f in "$@"; do
|
for f in "$@"; do
|
||||||
if [[ -f "$f" ]]; then
|
[[ -f "$f" ]] || continue
|
||||||
|
|
||||||
|
fm="$(awk '
|
||||||
|
{ sub(/\r$/, "") }
|
||||||
|
NR == 1 { sub(/^\357\273\277/, "") }
|
||||||
|
!opened && /^[[:blank:]]*$/ { next }
|
||||||
|
!opened {
|
||||||
|
if ($0 ~ /^---[[:blank:]]*$/) { opened = 1; next }
|
||||||
|
exit
|
||||||
|
}
|
||||||
|
/^---[[:blank:]]*$/ { closed = 1; exit }
|
||||||
|
{ print }
|
||||||
|
END { if (!opened || !closed) exit 3 }
|
||||||
|
' "$f")" || {
|
||||||
|
echo "ERROR: $f has no closing YAML frontmatter block (expected --- ... --- at the top of the file)"
|
||||||
|
exit 1
|
||||||
|
}
|
||||||
|
|
||||||
missing=""
|
missing=""
|
||||||
if ! grep -q "^name:" "$f"; then
|
printf '%s\n' "$fm" | grep -q "^name:" || missing="${missing}name: "
|
||||||
missing="${missing}name: "
|
printf '%s\n' "$fm" | grep -q "^description:" || missing="${missing}description: "
|
||||||
fi
|
|
||||||
if ! grep -q "^description:" "$f"; then
|
# Scoped to the `metadata:` block and stopped at the next
|
||||||
missing="${missing}description: "
|
# top-level key, so a `version:` under a following `source:` list
|
||||||
fi
|
# cannot stand in for it; the `^ version:` anchor is exact, so a
|
||||||
if ! grep -A10 "^metadata:" "$f" | grep -q " version:"; then
|
# deeper-nested ` version:` cannot either. No line budget, so a
|
||||||
missing="${missing}metadata.version "
|
# long `metadata:` block does not hide the key.
|
||||||
fi
|
ver="$(printf '%s\n' "$fm" | awk '
|
||||||
|
/^metadata:/ { inm = 1; next }
|
||||||
|
inm && /^[A-Za-z]/ { exit }
|
||||||
|
inm && /^ version:/ {
|
||||||
|
v = $0
|
||||||
|
sub(/^ version:[[:blank:]]*/, "", v)
|
||||||
|
sub(/[[:blank:]]+#.*$/, "", v)
|
||||||
|
sub(/[[:blank:]]+$/, "", v)
|
||||||
|
print "found:" v
|
||||||
|
exit
|
||||||
|
}
|
||||||
|
')"
|
||||||
|
[[ -n "$ver" ]] || missing="${missing}metadata.version "
|
||||||
|
|
||||||
if [[ -n "$missing" ]]; then
|
if [[ -n "$missing" ]]; then
|
||||||
echo "ERROR: $f is missing required frontmatter fields (${missing})"
|
echo "ERROR: $f is missing required frontmatter fields (${missing})"
|
||||||
exit 1
|
exit 1
|
||||||
fi
|
fi
|
||||||
|
|
||||||
|
raw="${ver#found:}"
|
||||||
|
v="$raw"
|
||||||
|
case "$v" in
|
||||||
|
\"*\") v="${v#\"}"; v="${v%\"}" ;;
|
||||||
|
\'*\') v="${v#\'}"; v="${v%\'}" ;;
|
||||||
|
esac
|
||||||
|
if [[ ! "$v" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
|
||||||
|
echo "ERROR: $f has a malformed frontmatter metadata.version (${raw:-<empty>}) -- expected a three-part semver, e.g. \"1.0.0\""
|
||||||
|
exit 1
|
||||||
fi
|
fi
|
||||||
done
|
done
|
||||||
|
# arg0 for `bash -c`. WITHOUT it pre-commit's first filename lands in
|
||||||
|
# $0 and is dropped from "$@" -- so a single-file commit, the normal
|
||||||
|
# case, ran the loop zero times and reported Passed having checked
|
||||||
|
# nothing. Do not remove; tests/test-skill-frontmatter.sh pins it.
|
||||||
|
- skill-frontmatter
|
||||||
|
|
||||||
- id: skill-size-check
|
- id: skill-size-check
|
||||||
stages: ['pre-commit']
|
stages: ['pre-commit']
|
||||||
|
|||||||
@@ -128,12 +128,43 @@ not an authoring change.
|
|||||||
### `skill-frontmatter`, the other hook on that scope
|
### `skill-frontmatter`, the other hook on that scope
|
||||||
|
|
||||||
A second `repo: local` pre-commit hook, `skill-frontmatter`, runs on the **same** `files:` pattern at
|
A second `repo: local` pre-commit hook, `skill-frontmatter`, runs on the **same** `files:` pattern at
|
||||||
the same stage. It is a short shell loop: for each file, `grep -q "^name:"` and
|
the same stage. It is a shell loop that, **for the YAML frontmatter block only** — everything between
|
||||||
`grep -q "^description:"`, failing with "missing required frontmatter fields" if either is absent.
|
the opening `---` and the next `---` — asserts four things per file:
|
||||||
|
|
||||||
**It overlaps ADR-0020's "description present and non-empty" FAIL, and the overlap is not clean.**
|
| Check | Rejects with |
|
||||||
The ADR (`:95-101`) requires that question be decided on the **YAML-folded value** and nowhere else,
|
|---|---|
|
||||||
precisely because a line regex gets it wrong in both directions. Measured on fixtures:
|
| a `^name:` line is present | "missing required frontmatter fields (name: …)" |
|
||||||
|
| a `^description:` line is present | "missing required frontmatter fields (description: …)" |
|
||||||
|
| `metadata:` contains a `^ version:` key, anchored, scanning to the next top-level key | "missing required frontmatter fields (metadata.version)" |
|
||||||
|
| that version's value is three-part semver (`1.0.0`, quoted or not) | "has a malformed frontmatter metadata.version (…)" |
|
||||||
|
|
||||||
|
Every one of those qualifiers is load-bearing, and each replaced a defect that let the hook report
|
||||||
|
Passed having measured nothing. `tests/test-skill-frontmatter.sh` pins all of them:
|
||||||
|
|
||||||
|
- **Frontmatter-scoped, not whole-file.** The checks used to `grep` the entire file, so a `metadata:`
|
||||||
|
or `name:` block quoted in a **body code fence** satisfied them — `skill-author`'s own docs quote
|
||||||
|
exactly such a block.
|
||||||
|
- **Bounded by the next top-level key, not by `-A10`.** The version check was
|
||||||
|
`grep -A10 "^metadata:" | grep -q " version:"`, which ran ten lines past the end of the block: a
|
||||||
|
`version:` belonging to a following `source:` list entry counted (`write-docs` and `research` both
|
||||||
|
have a `source:` list immediately after `metadata:`), while a `metadata:` block with more than ten
|
||||||
|
lines before its `version:` was reported missing.
|
||||||
|
- **`^ version:` anchored.** `" version:"` was an unanchored substring, so a deeper-nested
|
||||||
|
` version:` matched too.
|
||||||
|
- **The value is asserted, not just the key.** `plugins/bin/.apm/skills/write-docs/SKILL.md` carried
|
||||||
|
`version: "1.0"` — present, correctly nested, and not a version — through an entire PR under a
|
||||||
|
presence-only check. Two-part `1.0` is a YAML float, not a version string.
|
||||||
|
- **The call shape is pinned.** `entry: bash` with `args: ['-c', <script>, …]` needs an explicit
|
||||||
|
arg0 placeholder after the script: without it `bash -c` puts pre-commit's **first** filename in
|
||||||
|
`$0`, where `for f in "$@"` never sees it. A single-file commit — the normal case — therefore ran
|
||||||
|
the loop body zero times and exited 0. The third `args` entry (`skill-frontmatter`) exists solely
|
||||||
|
to absorb `$0`; do not remove it.
|
||||||
|
- **An unreadable file is an error, not a pass.** A file with no closing `---` fails with "no closing
|
||||||
|
YAML frontmatter block" rather than falling through to a green.
|
||||||
|
|
||||||
|
**It still overlaps ADR-0020's "description present and non-empty" FAIL, and the overlap is not
|
||||||
|
clean.** The ADR (`:95-101`) requires that question be decided on the **YAML-folded value** and
|
||||||
|
nowhere else, precisely because a line regex gets it wrong in both directions. Measured on fixtures:
|
||||||
|
|
||||||
| Frontmatter | `skill-frontmatter` | `skill-size-check` |
|
| Frontmatter | `skill-frontmatter` | `skill-size-check` |
|
||||||
|---|---|---|
|
|---|---|---|
|
||||||
@@ -145,10 +176,34 @@ against, and it is the only one of the two that objects to a quoted key. Neither
|
|||||||
currently live in the corpus, and the honest reading is that presence is `skill-size-check`'s
|
currently live in the corpus, and the honest reading is that presence is `skill-size-check`'s
|
||||||
question — the grep's contribution to it is noise on one shape and silence on the other.
|
question — the grep's contribution to it is noise on one shape and silence on the other.
|
||||||
|
|
||||||
What the grep does add is the `name:` key, which **no** ADR-0020 check reads: a `SKILL.md` with no
|
What the hook adds that **no** ADR-0020 check reads is two keys: `name:` and `metadata.version`. A
|
||||||
`name:` passes `skill-size-check` at exit 0. That is its real and only unique coverage, and the
|
`SKILL.md` missing either passes `skill-size-check` at exit 0. That is its unique coverage, and the
|
||||||
reason not to fold it into the size gate on the grounds of redundancy.
|
reason not to fold it into the size gate on the grounds of redundancy.
|
||||||
|
|
||||||
|
#### Why this one stays a shell parser
|
||||||
|
|
||||||
|
[`python3` and PyYAML are hard requirements](#python3-and-pyyaml-are-hard-requirements) below records
|
||||||
|
that a hand-rolled frontmatter reader on this exact `files:` scope was **deliberately deleted**,
|
||||||
|
because "a reader that mis-parses an unfamiliar scalar shape reports a clean pass on a file it never
|
||||||
|
measured." That reasoning is about `skill-size-check` and does **not** transfer here. Do not delete
|
||||||
|
this hook citing it. Three differences:
|
||||||
|
|
||||||
|
1. **It answers a strictly narrower question.** `skill-size-check` must know the *folded value* of a
|
||||||
|
`>`-block scalar to count its characters, which is where a line reader diverges from a parser —
|
||||||
|
one corpus description measured 270 characters parsed and 412 unparsed. This hook asks only
|
||||||
|
whether a key is on a line and whether one short **plain scalar** matches `N.N.N`. There is no
|
||||||
|
folding, no multi-line value, and no measurement to get subtly wrong.
|
||||||
|
2. **It is frontmatter-scoped.** The failure mode that killed the old fallback was silently reading
|
||||||
|
past or short of the block. This one extracts the block explicitly and errors out when it cannot
|
||||||
|
find a closing marker, so "could not parse" is a red, never a green.
|
||||||
|
3. **It is pinned by tests.** `tests/test-skill-frontmatter.sh` drives the hook through pre-commit's
|
||||||
|
real `bash -c <script> <arg0> <files…>` invocation and asserts each defect class above. The
|
||||||
|
deleted fallback had no such suite; that is how its disagreement with a real parser survived.
|
||||||
|
|
||||||
|
The trade it buys is that the hook stays repo-local. Moving it to a script would change the
|
||||||
|
externally exposed `.pre-commit-hooks.yaml` contract for consumers, for a check that has no need of a
|
||||||
|
YAML parser.
|
||||||
|
|
||||||
### Two independent gate families, neither replaced the other
|
### Two independent gate families, neither replaced the other
|
||||||
|
|
||||||
**Family 1 — agentskills.io spec backstop** (unchanged, conformance not quality):
|
**Family 1 — agentskills.io spec backstop** (unchanged, conformance not quality):
|
||||||
@@ -413,6 +468,15 @@ reader that mis-parses an unfamiliar scalar shape reports a clean pass on a file
|
|||||||
which is the exact vacuous-green failure the `python3` check exists to avoid. `pip install pyyaml`
|
which is the exact vacuous-green failure the `python3` check exists to avoid. `pip install pyyaml`
|
||||||
(or `python3 -m pip install PyYAML`, or the distro's `python3-yaml`) if the hook reports it missing.
|
(or `python3 -m pip install PyYAML`, or the distro's `python3-yaml`) if the hook reports it missing.
|
||||||
|
|
||||||
|
**Neither requirement generalises to every hook on this scope, and one deliberate exception sits
|
||||||
|
right next to it.** [`skill-frontmatter`](#skill-frontmatter-the-other-hook-on-that-scope) runs on the
|
||||||
|
same `files:` pattern as a **shell** parser, on purpose — it asks only whether a key is on a line and
|
||||||
|
whether one short plain scalar matches `N.N.N`, with no folding to get wrong, and moving it to a
|
||||||
|
script would change the externally exposed `.pre-commit-hooks.yaml` contract for consumers. That
|
||||||
|
section carries the full argument. A reader arriving here first should not read this one as
|
||||||
|
condemning it. `check-rtk-prefix` needs `python3` but **not** PyYAML: it reads the markdown body and
|
||||||
|
never touches frontmatter, so it has no scalar to fold.
|
||||||
|
|
||||||
## Agent files take the description gates, not the body gate
|
## Agent files take the description gates, not the body gate
|
||||||
|
|
||||||
`check-apm-agents-valid` runs agent-audit's `validate.sh` over every real
|
`check-apm-agents-valid` runs agent-audit's `validate.sh` over every real
|
||||||
|
|||||||
366
tests/test-skill-frontmatter.sh
Normal file
366
tests/test-skill-frontmatter.sh
Normal file
@@ -0,0 +1,366 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
# Regression test for the `skill-frontmatter` pre-commit hook.
|
||||||
|
#
|
||||||
|
# The hook is `entry: bash` with `args: ['-c', <script>, <arg0>]`, and this file
|
||||||
|
# drives that exact call shape -- read out of .pre-commit-config.yaml, never
|
||||||
|
# re-implemented here. That is the whole point. `grep -rn skill-frontmatter
|
||||||
|
# tests/` returned nothing before this file existed, and the two defect classes
|
||||||
|
# below are both invisible to a test that copies the script body and calls it
|
||||||
|
# some other way:
|
||||||
|
#
|
||||||
|
# 1. THE POSITIONAL DROP. `bash -c <script> fileA fileB` puts fileA in $0, not
|
||||||
|
# in "$@". The hook had no arg0 placeholder, so pre-commit's FIRST filename
|
||||||
|
# was swallowed -- and a single-file commit, the normal case, ran the loop
|
||||||
|
# body zero times and reported Passed having checked nothing. A test that
|
||||||
|
# sources or inlines the script never sees this; only the real invocation
|
||||||
|
# shape does. Cases 2 and 3 below are that regression.
|
||||||
|
#
|
||||||
|
# 2. THE UNSCOPED GREP. The checks used to run over the WHOLE file, and the
|
||||||
|
# version check was `grep -A10 "^metadata:" | grep -q " version:"`. Four
|
||||||
|
# confirmed ways to pass while measuring nothing, all pinned below:
|
||||||
|
# * `metadata:` in a BODY code fence satisfies it (skill-author's own
|
||||||
|
# docs quote exactly such a block);
|
||||||
|
# * `-A10` runs past the end of the metadata block, so a `version:`
|
||||||
|
# belonging to a following `source:` list entry satisfies it
|
||||||
|
# (write-docs and research both have `source:` right after `metadata:`);
|
||||||
|
# * `" version:"` is an unanchored substring, so ` version:` at a
|
||||||
|
# deeper nesting satisfies it;
|
||||||
|
# * and the mirror image, a false NEGATIVE: a metadata block with more
|
||||||
|
# than 10 lines before `version:` was reported missing.
|
||||||
|
#
|
||||||
|
# Plus the semver assertion, which is not redundant with presence: write-docs
|
||||||
|
# carried `version: "1.0"` -- present, well-nested, and not a version -- through
|
||||||
|
# an entire PR under a check that only ever asked whether the key was there.
|
||||||
|
set -euo pipefail
|
||||||
|
|
||||||
|
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
||||||
|
CONFIG="$REPO_ROOT/.pre-commit-config.yaml"
|
||||||
|
PASS=0
|
||||||
|
FAIL=0
|
||||||
|
|
||||||
|
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
||||||
|
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
||||||
|
|
||||||
|
# Same guard shape as tests/test-check-executables-allow-sync.sh: the hook
|
||||||
|
# definition is YAML and reading it any other way is guessing. README.md lists
|
||||||
|
# python3/PyYAML as a pre-push prerequisite, and `run-tests.sh --strict` turns
|
||||||
|
# this skip into a failure, which is the correct reading when it runs as a gate.
|
||||||
|
command -v python3 > /dev/null 2>&1 || { echo "SKIP: python3 is required to read the hook definition out of .pre-commit-config.yaml"; exit 77; }
|
||||||
|
python3 -c 'import yaml' > /dev/null 2>&1 || { echo "SKIP: PyYAML is required to read the hook definition out of .pre-commit-config.yaml"; exit 77; }
|
||||||
|
|
||||||
|
TMPDIR_T="$(mktemp -d)"
|
||||||
|
trap 'rm -rf "$TMPDIR_T"' EXIT
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# The hook, as pre-commit will run it
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# ENTRY and ARGS come straight out of the config. run_hook() then reproduces
|
||||||
|
# pre-commit's own composition -- entry, then args, then the filenames appended
|
||||||
|
# LAST -- so the positional handling under test is the real one.
|
||||||
|
HOOK_JSON="$TMPDIR_T/hook.json"
|
||||||
|
python3 - "$CONFIG" "$HOOK_JSON" <<'PY'
|
||||||
|
import json
|
||||||
|
import sys
|
||||||
|
|
||||||
|
import yaml
|
||||||
|
|
||||||
|
config_path, out_path = sys.argv[1:3]
|
||||||
|
with open(config_path, encoding='utf-8') as fh:
|
||||||
|
cfg = yaml.safe_load(fh) or {}
|
||||||
|
|
||||||
|
found = None
|
||||||
|
for repo in cfg.get('repos') or []:
|
||||||
|
for hook in repo.get('hooks') or []:
|
||||||
|
if hook.get('id') == 'skill-frontmatter':
|
||||||
|
found = hook
|
||||||
|
|
||||||
|
if found is None:
|
||||||
|
sys.exit("skill-frontmatter hook not found in " + config_path)
|
||||||
|
|
||||||
|
with open(out_path, 'w', encoding='utf-8') as fh:
|
||||||
|
json.dump(
|
||||||
|
{
|
||||||
|
'entry': found.get('entry'),
|
||||||
|
'args': found.get('args') or [],
|
||||||
|
'files': found.get('files'),
|
||||||
|
'pass_filenames': found.get('pass_filenames', True),
|
||||||
|
'always_run': found.get('always_run', False),
|
||||||
|
},
|
||||||
|
fh,
|
||||||
|
)
|
||||||
|
PY
|
||||||
|
|
||||||
|
# Reads NUL-delimited fields on stdin into the named array. bash 3.2 (macOS) has
|
||||||
|
# no `mapfil[e]`/`readarra[y]` builtin, and tests/test-vale-wrap.sh's static scan
|
||||||
|
# rejects both — the bracket classes above match what a bare spelling would while
|
||||||
|
# keeping one out of this file, the same trick that file uses for `npro[c]`.
|
||||||
|
read_nul_array() {
|
||||||
|
local __var="$1"
|
||||||
|
shift
|
||||||
|
local __item
|
||||||
|
eval "$__var=()"
|
||||||
|
while IFS= read -r -d '' __item; do
|
||||||
|
eval "$__var+=(\"\$__item\")"
|
||||||
|
done
|
||||||
|
}
|
||||||
|
|
||||||
|
ENTRY="$(python3 -c 'import json,sys; print(json.load(open(sys.argv[1]))["entry"])' "$HOOK_JSON")"
|
||||||
|
ARGS=()
|
||||||
|
read_nul_array ARGS < <(python3 -c '
|
||||||
|
import json
|
||||||
|
import sys
|
||||||
|
|
||||||
|
data = json.load(open(sys.argv[1]))
|
||||||
|
for arg in data["args"]:
|
||||||
|
sys.stdout.write(arg + "\0")
|
||||||
|
' "$HOOK_JSON")
|
||||||
|
|
||||||
|
RUN_OUT=""
|
||||||
|
RUN_RC=0
|
||||||
|
# run_hook <file>... -- exactly `entry args... files...`, pre-commit's ordering.
|
||||||
|
run_hook() {
|
||||||
|
RUN_RC=0
|
||||||
|
RUN_OUT="$("$ENTRY" ${ARGS[@]+"${ARGS[@]}"} "$@" 2>&1)" || RUN_RC=$?
|
||||||
|
}
|
||||||
|
|
||||||
|
# write_skill <path> <frontmatter-body> [markdown-body]
|
||||||
|
write_skill() {
|
||||||
|
local path="$1" frontmatter="$2" body="${3:-# Heading
|
||||||
|
|
||||||
|
Body text.}"
|
||||||
|
mkdir -p "$(dirname "$path")"
|
||||||
|
{
|
||||||
|
printf -- '---\n'
|
||||||
|
printf '%s\n' "$frontmatter"
|
||||||
|
printf -- '---\n\n'
|
||||||
|
printf '%s\n' "$body"
|
||||||
|
} > "$path"
|
||||||
|
}
|
||||||
|
|
||||||
|
VALID_FM='name: valid-skill
|
||||||
|
description: A skill whose frontmatter is complete and well formed.
|
||||||
|
metadata:
|
||||||
|
version: "1.0.0"'
|
||||||
|
|
||||||
|
assert_passes() { # <label> <file>...
|
||||||
|
local label="$1"
|
||||||
|
shift
|
||||||
|
run_hook "$@"
|
||||||
|
if [[ $RUN_RC -eq 0 ]]; then
|
||||||
|
pass "$label"
|
||||||
|
else
|
||||||
|
fail "$label -- expected exit 0, got $RUN_RC. Output: $RUN_OUT"
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
assert_fails_with() { # <label> <needle> <file>...
|
||||||
|
local label="$1" needle="$2"
|
||||||
|
shift 2
|
||||||
|
run_hook "$@"
|
||||||
|
if [[ $RUN_RC -eq 0 ]]; then
|
||||||
|
fail "$label -- exited 0, so the hook reported Passed having checked nothing. Output: $RUN_OUT"
|
||||||
|
elif [[ "$RUN_OUT" != *"$needle"* ]]; then
|
||||||
|
fail "$label -- exited $RUN_RC but the message lacked '$needle'. Output: $RUN_OUT"
|
||||||
|
else
|
||||||
|
pass "$label"
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 1. The call shape itself
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Asserted as a contract as well as behaviourally, because the behavioural
|
||||||
|
# symptom of losing arg0 is a GREEN run -- the least likely thing to be noticed.
|
||||||
|
echo "--- the hook passes an arg0 placeholder so pre-commit's filenames land in \"\$@\" ---"
|
||||||
|
if [[ "$ENTRY" != "bash" ]]; then
|
||||||
|
fail "entry is '$ENTRY', not bash -- the arg0 reasoning below assumes bash -c"
|
||||||
|
elif [[ ${#ARGS[@]} -lt 3 ]]; then
|
||||||
|
fail "args has ${#ARGS[@]} entries; a 'bash -c <script>' hook needs a third, the arg0 placeholder, or the first filename is dropped from \"\$@\""
|
||||||
|
elif [[ "${ARGS[0]}" != "-c" ]]; then
|
||||||
|
fail "args[0] is '${ARGS[0]}', not -c"
|
||||||
|
elif [[ "${ARGS[2]}" == -* ]]; then
|
||||||
|
fail "args[2] is '${ARGS[2]}', which bash will read as a flag rather than as \$0"
|
||||||
|
else
|
||||||
|
pass "args is [-c, <script>, '${ARGS[2]}'] -- the third entry becomes \$0 and every filename reaches \"\$@\""
|
||||||
|
fi
|
||||||
|
|
||||||
|
FILES_PATTERN="$(python3 -c 'import json,sys; print(json.load(open(sys.argv[1]))["files"] or "")' "$HOOK_JSON")"
|
||||||
|
PASS_FILENAMES="$(python3 -c 'import json,sys; print(json.load(open(sys.argv[1]))["pass_filenames"])' "$HOOK_JSON")"
|
||||||
|
echo ""
|
||||||
|
echo "--- the hook is filename-driven, so the loop is the only thing that ever runs ---"
|
||||||
|
if [[ "$PASS_FILENAMES" != "True" ]]; then
|
||||||
|
fail "pass_filenames is $PASS_FILENAMES; with no filenames the loop body never executes and the hook is a permanent no-op"
|
||||||
|
elif [[ "$FILES_PATTERN" != '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$' ]]; then
|
||||||
|
fail "files: is '$FILES_PATTERN', not the .apm/ skill scope -- ADR-0020's gates and this one must agree on scope"
|
||||||
|
else
|
||||||
|
pass "pass_filenames is on and files: is the .apm/ skill scope"
|
||||||
|
fi
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 2. A single bad file, on its own -- THE regression
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# This is the shipped bug in one line: one file, which is what a commit
|
||||||
|
# touching one skill hands the hook. Before the arg0 fix the file landed in $0,
|
||||||
|
# "$@" was empty, and this exited 0.
|
||||||
|
echo ""
|
||||||
|
echo "--- a SINGLE bad file fails (before the arg0 fix this exited 0 having read nothing) ---"
|
||||||
|
write_skill "$TMPDIR_T/single/SKILL.md" 'name: single
|
||||||
|
description: Missing its metadata block entirely.'
|
||||||
|
assert_fails_with "one file with no metadata block is rejected" "missing required frontmatter fields" "$TMPDIR_T/single/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- and a single GOOD file still passes, so the case above is not failing for some other reason ---"
|
||||||
|
write_skill "$TMPDIR_T/single-ok/SKILL.md" "$VALID_FM"
|
||||||
|
assert_passes "one valid file passes" "$TMPDIR_T/single-ok/SKILL.md"
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 3. Position within the argument list must not matter
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
echo ""
|
||||||
|
echo "--- the bad file is caught wherever it sits in the argument list ---"
|
||||||
|
write_skill "$TMPDIR_T/ok-a/SKILL.md" "$VALID_FM"
|
||||||
|
write_skill "$TMPDIR_T/ok-b/SKILL.md" "$VALID_FM"
|
||||||
|
write_skill "$TMPDIR_T/bad/SKILL.md" 'name: bad
|
||||||
|
description: No metadata block.'
|
||||||
|
|
||||||
|
assert_fails_with "bad file FIRST is caught" "$TMPDIR_T/bad/SKILL.md" \
|
||||||
|
"$TMPDIR_T/bad/SKILL.md" "$TMPDIR_T/ok-a/SKILL.md" "$TMPDIR_T/ok-b/SKILL.md"
|
||||||
|
assert_fails_with "bad file LAST is caught (the loop reaches the end of \"\$@\")" "$TMPDIR_T/bad/SKILL.md" \
|
||||||
|
"$TMPDIR_T/ok-a/SKILL.md" "$TMPDIR_T/ok-b/SKILL.md" "$TMPDIR_T/bad/SKILL.md"
|
||||||
|
assert_passes "three valid files pass" "$TMPDIR_T/ok-a/SKILL.md" "$TMPDIR_T/ok-b/SKILL.md" "$TMPDIR_T/single-ok/SKILL.md"
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 4. The four grep defects
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
echo ""
|
||||||
|
echo "--- defect 1: a \`metadata:\` block in a BODY code fence is documentation, not frontmatter ---"
|
||||||
|
# skill-author's docs quote a metadata block verbatim. Under the old whole-file
|
||||||
|
# grep that quotation satisfied the check for the file quoting it.
|
||||||
|
write_skill "$TMPDIR_T/fence/SKILL.md" 'name: fence
|
||||||
|
description: Frontmatter has no metadata block; the body quotes one.' '# Fence
|
||||||
|
|
||||||
|
Skills declare their version like this:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
metadata:
|
||||||
|
version: "1.0.0"
|
||||||
|
```'
|
||||||
|
assert_fails_with "a quoted metadata block in the body does not satisfy metadata.version" "missing required frontmatter fields" "$TMPDIR_T/fence/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- ... and the same for name: and description: quoted in the body ---"
|
||||||
|
write_skill "$TMPDIR_T/fence-keys/SKILL.md" 'metadata:
|
||||||
|
version: "1.0.0"' '# Fence keys
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
name: not-the-real-name
|
||||||
|
description: not the real description
|
||||||
|
```'
|
||||||
|
assert_fails_with "body-fenced name:/description: do not satisfy the presence checks" "name: description:" "$TMPDIR_T/fence-keys/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- defect 2: a \`version:\` under a following \`source:\` list is not metadata.version ---"
|
||||||
|
# `-A10` ran ten lines past `metadata:` regardless of where the block ended.
|
||||||
|
# write-docs and research both carry a `source:` list immediately after it.
|
||||||
|
write_skill "$TMPDIR_T/source-list/SKILL.md" 'name: source-list
|
||||||
|
description: metadata has no version; the next top-level key does.
|
||||||
|
metadata:
|
||||||
|
author: someone
|
||||||
|
source:
|
||||||
|
- name: upstream
|
||||||
|
version: "2.3.4"'
|
||||||
|
assert_fails_with "a version: belonging to source[] does not satisfy metadata.version" "missing required frontmatter fields" "$TMPDIR_T/source-list/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- defect 3: a deeper-nested \` version:\` is not metadata.version ---"
|
||||||
|
# `grep -q " version:"` was an unanchored substring match, so any indentation
|
||||||
|
# of two spaces or more matched.
|
||||||
|
write_skill "$TMPDIR_T/deep-indent/SKILL.md" 'name: deep-indent
|
||||||
|
description: The only version: key sits one level too deep.
|
||||||
|
metadata:
|
||||||
|
provenance:
|
||||||
|
version: "1.0.0"'
|
||||||
|
assert_fails_with "a four-space-indented version: does not satisfy metadata.version" "missing required frontmatter fields" "$TMPDIR_T/deep-indent/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- defect 4: a version: more than ten lines into the metadata block is FOUND ---"
|
||||||
|
# The mirror image: the old check reported this one missing.
|
||||||
|
write_skill "$TMPDIR_T/long-metadata/SKILL.md" 'name: long-metadata
|
||||||
|
description: A long metadata block whose version sits well past line ten.
|
||||||
|
metadata:
|
||||||
|
a: 1
|
||||||
|
b: 2
|
||||||
|
c: 3
|
||||||
|
d: 4
|
||||||
|
e: 5
|
||||||
|
f: 6
|
||||||
|
g: 7
|
||||||
|
h: 8
|
||||||
|
i: 9
|
||||||
|
j: 10
|
||||||
|
k: 11
|
||||||
|
version: "1.0.0"'
|
||||||
|
assert_passes "a version: 13 lines into the metadata block is found" "$TMPDIR_T/long-metadata/SKILL.md"
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 5. Present is not the same as well formed
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
echo ""
|
||||||
|
echo "--- a present-but-non-semver version is rejected, with its own message ---"
|
||||||
|
# plugins/bin/.apm/skills/write-docs/SKILL.md carried exactly this through a
|
||||||
|
# whole PR: the key was present, so a presence-only check had nothing to say.
|
||||||
|
write_skill "$TMPDIR_T/two-part/SKILL.md" 'name: two-part
|
||||||
|
description: Its version is two-part, which is a float in YAML, not a version.
|
||||||
|
metadata:
|
||||||
|
version: "1.0"'
|
||||||
|
assert_fails_with "\"1.0\" is rejected as malformed, not reported as missing" "malformed frontmatter metadata.version" "$TMPDIR_T/two-part/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- ... and the malformed message quotes the offending value ---"
|
||||||
|
run_hook "$TMPDIR_T/two-part/SKILL.md"
|
||||||
|
if [[ "$RUN_OUT" == *'"1.0"'* ]]; then
|
||||||
|
pass "the message names the value it rejected"
|
||||||
|
else
|
||||||
|
fail "the message did not quote the rejected value. Output: $RUN_OUT"
|
||||||
|
fi
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- other non-semver shapes ---"
|
||||||
|
write_skill "$TMPDIR_T/empty-version/SKILL.md" 'name: empty-version
|
||||||
|
description: The key is present with no value at all.
|
||||||
|
metadata:
|
||||||
|
version:'
|
||||||
|
assert_fails_with "a valueless version: is rejected" "metadata.version" "$TMPDIR_T/empty-version/SKILL.md"
|
||||||
|
|
||||||
|
write_skill "$TMPDIR_T/word-version/SKILL.md" 'name: word-version
|
||||||
|
description: A non-numeric version.
|
||||||
|
metadata:
|
||||||
|
version: latest'
|
||||||
|
assert_fails_with "version: latest is rejected" "malformed frontmatter metadata.version" "$TMPDIR_T/word-version/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- an unquoted three-part version is accepted (both YAML spellings are legal) ---"
|
||||||
|
write_skill "$TMPDIR_T/unquoted/SKILL.md" 'name: unquoted
|
||||||
|
description: An unquoted semver, which YAML reads as a string.
|
||||||
|
metadata:
|
||||||
|
version: 0.1.4'
|
||||||
|
assert_passes "version: 0.1.4 unquoted passes" "$TMPDIR_T/unquoted/SKILL.md"
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# 6. A file that cannot be read as frontmatter must not report green
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
echo ""
|
||||||
|
echo "--- a file with no frontmatter block fails loudly rather than passing vacuously ---"
|
||||||
|
printf '# Just a document\n\nname: not-frontmatter\ndescription: nor this\n' > "$TMPDIR_T/no-fm.md"
|
||||||
|
assert_fails_with "a file with no --- block is an error" "no closing YAML frontmatter block" "$TMPDIR_T/no-fm.md"
|
||||||
|
|
||||||
|
printf -- '---\nname: unterminated\ndescription: the block is never closed\nmetadata:\n version: "1.0.0"\n' > "$TMPDIR_T/unterminated.md"
|
||||||
|
assert_fails_with "an unterminated frontmatter block is an error" "no closing YAML frontmatter block" "$TMPDIR_T/unterminated.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "--- a path that does not exist is skipped, not crashed on ---"
|
||||||
|
assert_passes "a nonexistent path is ignored" "$TMPDIR_T/nope/SKILL.md"
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "Results: $PASS passed, $FAIL failed"
|
||||||
|
[[ $FAIL -eq 0 ]]
|
||||||
Reference in New Issue
Block a user