fix(kyberforge): close the vacuous-pass paths in the ADR-0020 gate scripts
Three ways the gates could report green having measured nothing. All three were
invisible to a passing test suite, because pre-commit prints nothing at all for a
hook that exits 0 — a gate that declines to check and a gate that checked and
passed produce the identical signal.
- A UTF-8 BOM, a leading blank line, a trailing space after a `---` marker or
CRLF line endings defeated the `^---\n` frontmatter matcher. Every ADR-0020
check was then skipped and the file passed: measured at the time, a
550-character description with a 1,000-word body exited 0 behind a BOM.
All four shapes are now tolerated, and frontmatter that genuinely cannot be
parsed is a hard ERROR rather than a silent skip.
- An agent file with a valueless `description:` followed by another key let a
line regex capture the *next* key, which looked non-empty, so the
missing-or-empty branch never fired and every gate below it early-returned on
the empty folded value — zero output, exit 0, on a blocking gate. The one
field this contract is entirely about was the one field a gate could fail to
notice was absent. Presence is now decided on the YAML-folded value and
nowhere else, and a missing or empty description is a hard FAIL in all three
validators.
- The hand-rolled frontmatter fallback disagreed with PyYAML across the FAIL
boundary on folded scalars, so which reader happened to be available decided
the verdict. A fallback that mis-parses a scalar shape reports a vacuous pass,
which is worse than not running, so it is deleted: python3 and PyYAML are hard
requirements that fail loudly with an install pointer.
Boundary-target resolution no longer derives its universe from its own location.
A `${BASH_SOURCE}`-relative repo root leaked this repo's 39-skill universe into
every consumer repo running the hook through pre-commit, so a consumer skill
routing to `skill-audit` resolved against a plugin it had never installed. The
interim form resolved through `.claude/` and `.agents/`, which are gitignored
`apm install` output — the same commit reported 2 dangling targets on a machine
that had run the install and 6 on a fresh clone. Resolution now walks up from the
file being checked to an authoring root (nearest ancestor holding
`plugins/*/.apm/{skills,agents}`, else the nearest `.git`, in two passes so a
nested `.git` cannot outrank a real monorepo root); the universe is every skill
and agent under `<root>/plugins/*/` plus the file's own apm package and that
package's declared `dependencies.apm`. Deployed trees are consulted only when no
authoring root exists at all — the consumer case. One commit now gets one verdict,
which a gate shipping hot with no baseline file has to.
Narrowed in the same pass: a routing target inferred from the prose boundary form
and corroborated by nothing else reports at SUGGESTION instead of blocking. A
blocking check with no escape hatch is the wrong trade when the inference from
prose is the weak part of it.
New deterministic checks, all previously untested or absent: every
`references/<file>.md` a body names must exist (ERROR — a broken pointer is not a
style opinion); a description with no boundary clause at all, a Gotchas section
over five entries, and a Gotchas section over 25% of the body are SUGGESTIONs.
Where no universe can be determined the target check prints `INFO ... DID NOT
RUN` rather than passing quietly. Each prose-scanning check needed its own
false-positive fix — a fenced example of a Gotchas section was being read as the
section itself — and those fixes are pinned rather than assumed.
The resolver is one block copied verbatim into all three scripts between
BEGIN/END markers, because a cache-installed plugin's scripts cannot read outside
their own plugin directory. Nothing asserted the copies were still identical; a
one-line edit to a single copy passed every constant-agreement assertion, since
constants are not what drifts.
Tests land here rather than in a later commit. The existing suites assert the old
behaviour and go red against these scripts, so splitting them would leave a commit
whose own `run-tests` pre-push gate fails in isolation.
Refs: ADR-0020
This commit is contained in:
365
tests/test-adr0020-differential.sh
Executable file
365
tests/test-adr0020-differential.sh
Executable file
@@ -0,0 +1,365 @@
|
||||
#!/usr/bin/env bash
|
||||
# Differential test: scripts/skill-size-check.sh (the pre-commit hook) and
|
||||
# skill-audit/scripts/validate.sh (the in-skill auditor) must reach the SAME
|
||||
# ADR-0020 verdict on the same file.
|
||||
#
|
||||
# Why this exists as a separate suite. tests/test-skill-size-check.sh already
|
||||
# asserts the two agree on their CONSTANTS, and that assertion is necessary but
|
||||
# demonstrably not sufficient: a previous review found the two scripts disagreeing
|
||||
# on real files while every constant matched perfectly. Constants are one of the
|
||||
# ways two hand-duplicated implementations diverge; comparison operators, message
|
||||
# wording, which value gets measured, and which branch runs first are the others,
|
||||
# and none of them is visible to a constant check.
|
||||
#
|
||||
# The consequence of divergence is specific and bad: skill-audit reports a skill
|
||||
# ready to ship and the commit hook then rejects it, or worse, the reverse. So the
|
||||
# comparison here is over VERDICTS on files, not over source text.
|
||||
#
|
||||
# Scope: the ADR-0020 axes the two scripts share — description length and tier,
|
||||
# body word count and tier, dangling routing targets, missing references/
|
||||
# pointers, the two Gotchas suggestions, the missing-boundary-clause suggestion,
|
||||
# a declined resolution, and an empty description. The two scripts legitimately
|
||||
# differ elsewhere (validate.sh also checks name/directory agreement, script
|
||||
# executability and the 1024-char spec backstop; the hook checks whole-file lines
|
||||
# and words), and those lines are ignored rather than being forced into a shared
|
||||
# shape they were never meant to have.
|
||||
#
|
||||
# Run over the real 39-skill corpus AND over purpose-built fixtures that sit ON
|
||||
# each boundary. The corpus alone is not enough — it happens not to contain a
|
||||
# file at exactly 900 body words, which is precisely where an inclusive/exclusive
|
||||
# comparison mismatch would hide.
|
||||
set -euo pipefail
|
||||
|
||||
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
||||
HOOK="$REPO_ROOT/scripts/skill-size-check.sh"
|
||||
SKILL_VALIDATE="$REPO_ROOT/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh"
|
||||
|
||||
TMPDIR_T="$(mktemp -d)"
|
||||
trap 'rm -rf "$TMPDIR_T"' EXIT
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fixtures: one per ADR-0020 axis, placed ON the boundary wherever there is one.
|
||||
# ---------------------------------------------------------------------------
|
||||
# Built inside a synthetic plugin monorepo so boundary-target resolution actually
|
||||
# runs for both scripts (in a bare temp dir both would decline, and "both
|
||||
# declined" is agreement about nothing).
|
||||
FIXTURE_ROOT="$TMPDIR_T/fixtures"
|
||||
FX="$FIXTURE_ROOT/plugins/fixture-plugin/.apm/skills"
|
||||
mkdir -p "$FX/sibling-skill" "$FIXTURE_ROOT/plugins/fixture-plugin/.apm/agents"
|
||||
|
||||
make_fx() {
|
||||
local name="$1" desc="$2" body_words="$3"
|
||||
mkdir -p "$FX/$name"
|
||||
{
|
||||
echo "---"
|
||||
echo "name: $name"
|
||||
echo "description: $desc"
|
||||
echo "---"
|
||||
echo ""
|
||||
python3 -c "print(' '.join(['word'] * $body_words))"
|
||||
} > "$FX/$name/SKILL.md"
|
||||
}
|
||||
desc_of_length() {
|
||||
python3 - "$1" <<'PY'
|
||||
import sys
|
||||
n = int(sys.argv[1])
|
||||
prefix = 'Use when doing the thing. Do not use for anything else. '
|
||||
print(prefix + 'x' * (n - len(prefix)))
|
||||
PY
|
||||
}
|
||||
CLEAN="Use when doing the thing. Do not use for anything else."
|
||||
|
||||
# Description tier boundaries, both sides of both thresholds.
|
||||
make_fx desc-249 "$(desc_of_length 249)" 10
|
||||
make_fx desc-250 "$(desc_of_length 250)" 10
|
||||
make_fx desc-251 "$(desc_of_length 251)" 10
|
||||
make_fx desc-400 "$(desc_of_length 400)" 10
|
||||
make_fx desc-401 "$(desc_of_length 401)" 10
|
||||
# Body tier boundaries, both sides of both thresholds.
|
||||
make_fx body-599 "$CLEAN" 599
|
||||
make_fx body-600 "$CLEAN" 600
|
||||
make_fx body-601 "$CLEAN" 601
|
||||
make_fx body-900 "$CLEAN" 900
|
||||
make_fx body-901 "$CLEAN" 901
|
||||
# Folding: the value has to be measured after YAML folding in both scripts.
|
||||
mkdir -p "$FX/folded-desc"
|
||||
{
|
||||
echo "---"
|
||||
echo "name: folded-desc"
|
||||
echo "description: >"
|
||||
python3 -c "print('\n'.join([' ' + 'x' * 40] * 11))"
|
||||
echo "---"
|
||||
echo ""
|
||||
echo "Do the thing."
|
||||
} > "$FX/folded-desc/SKILL.md"
|
||||
# Routing targets, one per tier the resolver can produce: resolves, dangles with
|
||||
# in-sentence corroboration (ERROR/FAIL), dangles alone (SUGGESTION on both
|
||||
# sides), route notation (ERROR/FAIL without corroboration), attributive
|
||||
# (silent). Each tier is here because the two scripts have to agree on the TIER,
|
||||
# not merely on the finding — a copy that promoted or demoted one of them would
|
||||
# otherwise pass this comparison.
|
||||
make_fx target-resolves "Use when doing the thing. Do not use for the other thing — use sibling-skill instead." 10
|
||||
make_fx target-dangles "Use when doing the thing. Do not use for the other thing — use sibling-skill or no-such-skill instead." 10
|
||||
make_fx target-dangles-lone "Use when doing the thing. Do not use for the other thing — use no-such-lone-skill instead." 10
|
||||
make_fx target-dangles-notation "Use when doing the thing. Do not use for the other thing — use /no-such-notation-skill instead." 10
|
||||
make_fx target-attributive "Use when doing the thing. Use pre-commit hooks instead of ad-hoc scripts." 10
|
||||
# No boundary clause at all.
|
||||
make_fx no-boundary "Use when the user wants the thing done." 10
|
||||
# A missing references/ pointer, and a present one.
|
||||
make_fx ref-missing "$CLEAN" 10
|
||||
printf '\nIf the caller needs detail, read references/absent.md first.\n' >> "$FX/ref-missing/SKILL.md"
|
||||
make_fx ref-present "$CLEAN" 10
|
||||
printf '\nIf the caller needs detail, read references/there.md first.\n' >> "$FX/ref-present/SKILL.md"
|
||||
mkdir -p "$FX/ref-present/references"
|
||||
echo "detail" > "$FX/ref-present/references/there.md"
|
||||
# Gotchas, over each guideline.
|
||||
make_fx gotchas-many "$CLEAN" 0
|
||||
cat >> "$FX/gotchas-many/SKILL.md" <<'EOF'
|
||||
|
||||
## Gotchas
|
||||
|
||||
- one trap here
|
||||
- two trap here
|
||||
- three trap here
|
||||
- four trap here
|
||||
- five trap here
|
||||
- six trap here
|
||||
- seven trap here
|
||||
|
||||
## Notes
|
||||
|
||||
EOF
|
||||
python3 -c "print(' '.join(['word'] * 200))" >> "$FX/gotchas-many/SKILL.md"
|
||||
# Gotchas over the 25% body-fraction guideline. Prose, not list items, so the
|
||||
# entry guideline cannot fire and the two suggestions stay separable: 26 section
|
||||
# words in a 100-word body is one word past the threshold.
|
||||
make_fx gotchas-fraction "$CLEAN" 0
|
||||
{
|
||||
echo ""
|
||||
echo "## Gotchas"
|
||||
echo ""
|
||||
python3 -c "print(' '.join(['word'] * 26))"
|
||||
echo ""
|
||||
echo "## Notes"
|
||||
echo ""
|
||||
python3 -c "print(' '.join(['word'] * 70))"
|
||||
} >> "$FX/gotchas-fraction/SKILL.md"
|
||||
|
||||
# Empty description — the shape that used to exit 0 in silence.
|
||||
mkdir -p "$FX/empty-desc"
|
||||
printf -- '---\nname: empty-desc\ndescription:\nmodel: sonnet\n---\n\nDo the thing.\n' \
|
||||
> "$FX/empty-desc/SKILL.md"
|
||||
# Every boundary shape at once, so a divergence that only appears when several
|
||||
# findings fire together is not missed.
|
||||
make_fx combined "$(desc_of_length 401)" 901
|
||||
printf '\nIf the caller needs detail, read references/absent.md first.\n' >> "$FX/combined/SKILL.md"
|
||||
|
||||
# A skill with routing targets and NO authoring root above it — deliberately
|
||||
# OUTSIDE the fixture plugin tree. Both scripts must decline out loud, and both
|
||||
# must decline identically; "both declined" is only meaningful as agreement if
|
||||
# the declining path is exercised on purpose somewhere.
|
||||
ORPHAN_ROOT="$TMPDIR_T/orphan"
|
||||
mkdir -p "$ORPHAN_ROOT/no-universe"
|
||||
{
|
||||
echo "---"
|
||||
echo "name: no-universe"
|
||||
echo "description: Use when doing the thing. Do not use for the other thing — use some-other-skill instead."
|
||||
echo "---"
|
||||
echo ""
|
||||
echo "Do the thing."
|
||||
} > "$ORPHAN_ROOT/no-universe/SKILL.md"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The comparison
|
||||
# ---------------------------------------------------------------------------
|
||||
python3 - "$HOOK" "$SKILL_VALIDATE" "$REPO_ROOT" "$FX" "$ORPHAN_ROOT" <<'PYTHON'
|
||||
import glob
|
||||
import os
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
hook, validate, repo_root, fixture_dir, orphan_dir = sys.argv[1:6]
|
||||
|
||||
passes = 0
|
||||
failures = 0
|
||||
|
||||
|
||||
def ok(msg):
|
||||
global passes
|
||||
passes += 1
|
||||
print(" PASS: %s" % msg)
|
||||
|
||||
|
||||
def bad(msg):
|
||||
global failures
|
||||
failures += 1
|
||||
print(" FAIL: %s" % msg)
|
||||
|
||||
|
||||
# Tier prefixes. The hook writes `ERROR: ` / `SUGGESTION: ` / `INFO: `; the
|
||||
# auditor writes `FAIL ` / `SUGGESTION ` / `INFO ` and additionally `PASS `
|
||||
# lines, which carry no finding and are dropped.
|
||||
TIERS = (
|
||||
('ERROR', ('ERROR:', 'FAIL ')),
|
||||
('SUGGESTION', ('SUGGESTION:', 'SUGGESTION ')),
|
||||
('INFO', ('INFO:', 'INFO ')),
|
||||
)
|
||||
|
||||
# Each rule turns a finding line into a canonical token. Wording differs between
|
||||
# the two scripts by design (one addresses a committer, the other an auditor), so
|
||||
# the tokens deliberately capture the MEASUREMENT and not the sentence.
|
||||
RULES = (
|
||||
('DESC_CHARS', re.compile(r'description is (\d+) char')),
|
||||
('BODY_WORDS', re.compile(r'body is (\d+) words')),
|
||||
('ROUTE', re.compile(r"routes to '([^']+)'")),
|
||||
('MISSING_REF', re.compile(r'points at (references/[^\s,]+)')),
|
||||
('GOTCHA_ENTRIES', re.compile(r'Gotchas section has (\d+) entries')),
|
||||
('GOTCHA_FRACTION', re.compile(r'Gotchas section is (\d+) of (\d+) body words')),
|
||||
('NO_BOUNDARY_CLAUSE', re.compile(r'(description has no boundary clause)')),
|
||||
('RESOLUTION_DECLINED', re.compile(r'(boundary-target resolution DID NOT RUN)')),
|
||||
('DESC_EMPTY', re.compile(r'(description field is missing or empty)')),
|
||||
)
|
||||
|
||||
|
||||
def verdict(output):
|
||||
"""The set of ADR-0020 findings in a script's output, tier included.
|
||||
|
||||
Lines that match no rule are dropped rather than compared: the two scripts
|
||||
legitimately check different things outside ADR-0020 (name/directory
|
||||
agreement, script executability, the 1024-char spec backstop, whole-file
|
||||
line and word ceilings), and forcing those into the comparison would report
|
||||
a difference that is not a disagreement.
|
||||
"""
|
||||
found = set()
|
||||
for raw in output.splitlines():
|
||||
line = raw.strip()
|
||||
tier = None
|
||||
for name, prefixes in TIERS:
|
||||
if any(line.startswith(p) for p in prefixes):
|
||||
tier = name
|
||||
break
|
||||
if tier is None:
|
||||
continue
|
||||
for token, pattern in RULES:
|
||||
match = pattern.search(line)
|
||||
if match:
|
||||
found.add((tier, token) + tuple(match.groups()))
|
||||
return found
|
||||
|
||||
|
||||
def run(cmd):
|
||||
proc = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
|
||||
return proc.returncode, proc.stdout.decode('utf-8', 'replace')
|
||||
|
||||
|
||||
def compare(label, skill_dir):
|
||||
skill_md = os.path.join(skill_dir, 'SKILL.md')
|
||||
hook_rc, hook_out = run(['bash', hook, skill_md])
|
||||
audit_rc, audit_out = run(['bash', validate, skill_dir])
|
||||
hook_v = verdict(hook_out)
|
||||
audit_v = verdict(audit_out)
|
||||
|
||||
problems = []
|
||||
only_hook = sorted(hook_v - audit_v)
|
||||
only_audit = sorted(audit_v - hook_v)
|
||||
if only_hook:
|
||||
problems.append('only the hook reported %s' % (only_hook,))
|
||||
if only_audit:
|
||||
problems.append('only skill-audit reported %s' % (only_audit,))
|
||||
|
||||
# Exit codes are compared on the ADR-0020 axis only: an ERROR-tier ADR-0020
|
||||
# finding must make BOTH scripts non-zero, and neither may be turned
|
||||
# non-zero by a SUGGESTION. The raw codes cannot be compared directly —
|
||||
# validate.sh also fails on checks the hook does not run at all.
|
||||
hook_err = any(t == 'ERROR' for t, *_ in hook_v)
|
||||
audit_err = any(t == 'ERROR' for t, *_ in audit_v)
|
||||
if hook_err and hook_rc == 0:
|
||||
problems.append('the hook reported an ADR-0020 ERROR but exited 0')
|
||||
if audit_err and audit_rc == 0:
|
||||
problems.append('skill-audit reported an ADR-0020 FAIL but exited 0')
|
||||
if not hook_err and hook_rc != 0 and not _non_adr_hook_error(hook_out):
|
||||
problems.append('the hook exited %d with no ADR-0020 ERROR and no spec-ceiling ERROR'
|
||||
% hook_rc)
|
||||
|
||||
if problems:
|
||||
bad('%s: %s' % (label, '; '.join(problems)))
|
||||
else:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _non_adr_hook_error(output):
|
||||
"""True if the hook failed on a spec ceiling rather than an ADR-0020 gate.
|
||||
|
||||
MAX_LINES / MAX_WORDS are the hook's other ERROR sources and are outside
|
||||
this comparison, so a non-zero exit explained by one of them is not a
|
||||
disagreement.
|
||||
"""
|
||||
return bool(re.search(r'ERROR: .*(-line ceiling|-word ceiling \(~5,000 tokens)', output))
|
||||
|
||||
|
||||
# --- The real corpus -------------------------------------------------------
|
||||
corpus = sorted(glob.glob(os.path.join(repo_root, 'plugins', '*', '.apm', 'skills', '*')))
|
||||
corpus = [d for d in corpus if os.path.isfile(os.path.join(d, 'SKILL.md'))]
|
||||
print("")
|
||||
print("--- the two scripts agree on every skill in the live corpus (%d files) ---" % len(corpus))
|
||||
if len(corpus) < 30:
|
||||
bad('only %d corpus skills were discovered — the glob is wrong, so this leg '
|
||||
'proves nothing' % len(corpus))
|
||||
else:
|
||||
ok('discovered %d corpus skills to compare' % len(corpus))
|
||||
agreed = 0
|
||||
for skill_dir in corpus:
|
||||
rel = os.path.relpath(skill_dir, repo_root)
|
||||
if compare(rel, skill_dir):
|
||||
agreed += 1
|
||||
if agreed == len(corpus):
|
||||
ok('all %d corpus skills produce identical ADR-0020 verdicts from both scripts' % agreed)
|
||||
|
||||
# --- Boundary fixtures -----------------------------------------------------
|
||||
fixtures = sorted(d for d in (glob.glob(os.path.join(fixture_dir, '*'))
|
||||
+ glob.glob(os.path.join(orphan_dir, '*')))
|
||||
if os.path.isfile(os.path.join(d, 'SKILL.md')))
|
||||
print("")
|
||||
print("--- the two scripts agree on every boundary fixture (%d files) ---" % len(fixtures))
|
||||
if len(fixtures) < 15:
|
||||
bad('only %d fixtures were built — the fixture set is incomplete, so the '
|
||||
'boundaries the corpus does not cover are untested' % len(fixtures))
|
||||
else:
|
||||
ok('built %d boundary fixtures to compare' % len(fixtures))
|
||||
fx_agreed = 0
|
||||
for skill_dir in fixtures:
|
||||
if compare(os.path.basename(skill_dir), skill_dir):
|
||||
fx_agreed += 1
|
||||
if fx_agreed == len(fixtures):
|
||||
ok('all %d boundary fixtures produce identical ADR-0020 verdicts from both scripts' % fx_agreed)
|
||||
|
||||
# --- The comparison must not be vacuous ------------------------------------
|
||||
# Everything above would also pass if verdict() extracted nothing at all. So the
|
||||
# fixtures are required to have produced findings across every axis this suite
|
||||
# claims to compare — if a rule stops matching (a reworded message, say), that is
|
||||
# a silent loss of coverage and it fails here instead.
|
||||
print("")
|
||||
print("--- the comparison actually extracted findings on every axis it claims to cover ---")
|
||||
seen_tokens = set()
|
||||
for skill_dir in fixtures:
|
||||
_, out = run(['bash', hook, os.path.join(skill_dir, 'SKILL.md')])
|
||||
for entry in verdict(out):
|
||||
seen_tokens.add(entry[1])
|
||||
_, out = run(['bash', validate, skill_dir])
|
||||
for entry in verdict(out):
|
||||
seen_tokens.add(entry[1])
|
||||
expected_tokens = {t for t, _ in RULES}
|
||||
missing = sorted(expected_tokens - seen_tokens)
|
||||
if missing:
|
||||
bad('no fixture produced a finding for %s — verdict() may no longer match '
|
||||
'those messages, and any disagreement on them would go unseen' % missing)
|
||||
else:
|
||||
ok('every one of the %d compared axes was exercised by at least one fixture'
|
||||
% len(expected_tokens))
|
||||
|
||||
print("")
|
||||
print("Results: %d passed, %d failed" % (passes, failures))
|
||||
sys.exit(1 if failures else 0)
|
||||
PYTHON
|
||||
Reference in New Issue
Block a user