fix(gates): close six PR #135 review findings in gates and their tests
B1: check-skill-version-bump.sh resolves every merge-base with `git merge-base
--all` instead of the single base git happens to pick. A criss-cross history has
two, so the verdict turned on that choice: a skill byte-identical to main's tip
could still be reported "not above merge-base" / "not above main tip" and fail a
push that should pass. A skill now counts as changed only when it differs from
EVERY base, and its version must exceed the version at every base it exists at
as well as at the main tip; with more than one base the failure names which one.
Case 40 in tests/test-skill-version-bump.sh builds the criss-cross fixture and
pins both directions.
B2: check-apm-current.sh no longer assumes the remote default branch is `main`
when origin/HEAD is unset. A checkout whose default is `master` was standing on
its default branch and being told "this is a feature branch, so discard it" --
to throw away a real lock update. With origin/HEAD unset nothing is asserted and
the neutral advice stands. tests/test-apm-current-hook.sh covers the unset case
on both `main` and `master`.
#4: the required-frontmatter checks folded into skill-size-check.sh by c8a7c9e
were untested apart from the leading-zero shape -- mutating the missing-version
ERROR into a no-op left every suite green. tests/test-adr0020-frontmatter.sh now
pins name presence and non-emptiness, metadata.version presence and semver
shape, and the four grep defects the deleted test-skill-frontmatter.sh named.
#5: nothing asked whether a Vale rule still MATCHES anything -- rewriting
CompositionNote.yml's tokens to match nothing left test-vale-wrap.sh at 63/63.
Case 35 enumerates the rule files under the Kyberforge* style directories at run
time, requires an alert from each on its own fixture, and fails when a
discovered rule has no fixture row. The stale comment at case 31 is corrected.
#6: tests/run-tests.sh --strict exited 0 when discovery found no test-*.sh at
all; strictness only ever acted on skips, and with no suites there were none. It
now cross-checks the git index the way run-bats.sh does and fails
unconditionally on an empty set, naming the search root.
N9: the skill-size-check hook description in .pre-commit-config.yaml covered
only the size, context-budget and boundary-target gates. It now also names the
required frontmatter fields, matching docs/spec/gates.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NwD8Egs5r4ndqeFLmhusX2
This commit is contained in:
@@ -441,6 +441,231 @@ else
|
||||
fail "validate.sh agent mode exited $SILENT_RC with output '${SILENT_OUT:-<empty>}' — the original defect was exit 0 and total silence on a blocking pre-push gate"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 3. The REQUIRED-FIELD checks, folded in from the deleted `skill-frontmatter`
|
||||
# ---------------------------------------------------------------------------
|
||||
# Commit c8a7c9e retired the standalone `skill-frontmatter` hook and moved its
|
||||
# two presence checks — `name` non-empty, `metadata.version` present and
|
||||
# three-part semver — into skill-size-check.sh, beside the ADR-0020 gates. The
|
||||
# hook's own suite went with it, and only the leading-zero shape was left
|
||||
# covered (tests/test-skill-size-check.sh). Measured: mutating the
|
||||
# missing-version ERROR to a no-op left every suite in the repo green. These
|
||||
# cases are that behaviour pinned back down, on the same fixtures the deleted
|
||||
# suite used.
|
||||
#
|
||||
# Probed against the HOOK alone, deliberately. validate.sh's skill mode has its
|
||||
# own metadata.version check with its own wording, and its agent mode has none
|
||||
# at all — ADR-0022 binds skills, not agents — so probe_all's "all three must
|
||||
# agree" contract does not hold for this family and asserting it would be
|
||||
# asserting something the ADRs contradict.
|
||||
#
|
||||
# The four grep defects the deleted suite named are kept as cases because the
|
||||
# YAML-parsed implementation must not regress into any of them: a `metadata:`
|
||||
# block quoted in the BODY, a `version:` under a following `source:` list, a
|
||||
# deeper-indented `version:`, and the mirror image — a `version:` far down a
|
||||
# long metadata block, which the old `-A10` grep reported MISSING.
|
||||
|
||||
# write_required <name> <frontmatter> [body] — a SKILL.md whose only interesting
|
||||
# property is its frontmatter. The description and body sit well inside every
|
||||
# ADR-0020 ceiling, so a finding here is the required-field check and nothing
|
||||
# else; a fixture that also tripped a ceiling would satisfy "exits non-zero" for
|
||||
# the wrong reason.
|
||||
write_required() {
|
||||
local name="$1" frontmatter="$2" body="${3:-Body text.}"
|
||||
local dir="$TMPDIR_T/required/$name"
|
||||
mkdir -p "$dir"
|
||||
{
|
||||
printf -- '---\n'
|
||||
printf '%s\n' "$frontmatter"
|
||||
printf -- '---\n\n'
|
||||
printf '%s\n' "$body"
|
||||
} > "$dir/SKILL.md"
|
||||
printf '%s' "$dir/SKILL.md"
|
||||
}
|
||||
|
||||
# write_required_raw <name> <whole-file> — for the shapes that must NOT have a
|
||||
# closing marker written for them.
|
||||
write_required_raw() {
|
||||
local name="$1"
|
||||
local dir="$TMPDIR_T/required/$name"
|
||||
mkdir -p "$dir"
|
||||
printf '%s' "$2" > "$dir/SKILL.md"
|
||||
printf '%s' "$dir/SKILL.md"
|
||||
}
|
||||
|
||||
REQ_DESC='Use when probing the required-field checks. Do not use for anything else.'
|
||||
|
||||
# require_finding <label> <file> <needle> — non-zero exit AND the named message.
|
||||
# The needle is the message, not the exit code: the mutation this case exists to
|
||||
# catch turns the ERROR into a no-op, and a file that also failed some other gate
|
||||
# would still exit non-zero with the check gone.
|
||||
require_finding() {
|
||||
local label="$1" file="$2" needle="$3" out status=0
|
||||
set +e
|
||||
out="$(bash "$HOOK" "$file" 2>&1)"
|
||||
status=$?
|
||||
set -e
|
||||
if [[ $status -eq 0 ]]; then
|
||||
fail "$label — the hook exited 0: ${out:-<no output>}"
|
||||
elif [[ "$out" != *"$needle"* ]]; then
|
||||
fail "$label — the hook failed but never said '$needle': $out"
|
||||
else
|
||||
pass "$label"
|
||||
fi
|
||||
}
|
||||
|
||||
# require_clean <label> <file> — exits 0 with no finding at all.
|
||||
require_clean() {
|
||||
local label="$1" file="$2" out status=0
|
||||
set +e
|
||||
out="$(bash "$HOOK" "$file" 2>&1)"
|
||||
status=$?
|
||||
set -e
|
||||
if [[ $status -eq 0 ]]; then
|
||||
pass "$label"
|
||||
else
|
||||
fail "$label — expected exit 0, got $status: ${out:-<no output>}"
|
||||
fi
|
||||
}
|
||||
|
||||
echo ""
|
||||
echo "--- control: a SKILL.md carrying both required fields passes ---"
|
||||
# Without this, every case below could be passing because the fixture generator
|
||||
# is broken rather than because the checks fire.
|
||||
require_clean "a well-formed name + metadata.version passes" \
|
||||
"$(write_required valid "name: valid
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version: \"1.0.0\"")"
|
||||
|
||||
echo ""
|
||||
echo "--- metadata.version missing, in each of the shapes that used to satisfy the old grep ---"
|
||||
require_finding "no metadata block at all is reported missing" \
|
||||
"$(write_required no-metadata "name: no-metadata
|
||||
description: $REQ_DESC")" \
|
||||
"metadata.version field is missing"
|
||||
require_finding "a metadata block with other keys but no version is reported missing" \
|
||||
"$(write_required metadata-no-version "name: metadata-no-version
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
author: someone")" \
|
||||
"metadata.version field is missing"
|
||||
# `version:` with no value parses to None, which is absent, not malformed —
|
||||
# reporting it as a bad VALUE would send the author looking for a typo in a
|
||||
# value that is not there.
|
||||
require_finding "a valueless 'version:' is reported missing, not malformed" \
|
||||
"$(write_required empty-version "name: empty-version
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version:")" \
|
||||
"metadata.version field is missing"
|
||||
# skill-author's own docs quote a metadata block verbatim; under the old
|
||||
# whole-file grep that quotation satisfied the check for the file quoting it.
|
||||
require_finding "a metadata block quoted in the BODY does not satisfy the check" \
|
||||
"$(write_required fenced-metadata "name: fenced-metadata
|
||||
description: $REQ_DESC" '# Fenced
|
||||
|
||||
Skills declare their version like this:
|
||||
|
||||
```yaml
|
||||
metadata:
|
||||
version: "1.0.0"
|
||||
```')" \
|
||||
"metadata.version field is missing"
|
||||
# `-A10` ran ten lines past `metadata:` regardless of where the block ended, and
|
||||
# write-docs and research both carry a `source:` list immediately after it.
|
||||
require_finding "a version: belonging to a following source[] does not satisfy the check" \
|
||||
"$(write_required source-list "name: source-list
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
author: someone
|
||||
source:
|
||||
- name: upstream
|
||||
version: \"2.3.4\"")" \
|
||||
"metadata.version field is missing"
|
||||
# `grep -q \" version:\"` was an unanchored substring match, so any indentation
|
||||
# of two spaces or more matched.
|
||||
require_finding "a four-space-indented version: one level deeper does not satisfy the check" \
|
||||
"$(write_required deep-indent "name: deep-indent
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
provenance:
|
||||
version: \"1.0.0\"")" \
|
||||
"metadata.version field is missing"
|
||||
# The mirror image, and the reason this one asserts a PASS: the old grep's
|
||||
# ten-line window reported a real version missing once the block grew past it.
|
||||
require_clean "a version: thirteen lines into the metadata block is found" \
|
||||
"$(write_required long-metadata "name: long-metadata
|
||||
description: $REQ_DESC
|
||||
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\"")"
|
||||
|
||||
echo ""
|
||||
echo "--- present is not well formed: a non-semver metadata.version is its own finding ---"
|
||||
# plugins/bin/.apm/skills/write-docs/SKILL.md carried `version: "1.0"` through a
|
||||
# whole PR under a presence-only check: present, well-nested, and not a version.
|
||||
# The value is quoted back so the author does not have to guess which key.
|
||||
require_finding "'1.0' is rejected as malformed and the message quotes it" \
|
||||
"$(write_required two-part "name: two-part
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version: \"1.0\"")" \
|
||||
"metadata.version is malformed ('1.0')"
|
||||
require_finding "'latest' is rejected as malformed and the message quotes it" \
|
||||
"$(write_required word-version "name: word-version
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version: latest")" \
|
||||
"metadata.version is malformed ('latest')"
|
||||
|
||||
echo ""
|
||||
echo "--- the name field is required and must not be empty ---"
|
||||
require_finding "an absent name is reported" \
|
||||
"$(write_required no-name "description: $REQ_DESC
|
||||
metadata:
|
||||
version: \"1.0.0\"")" \
|
||||
"name field is missing or empty"
|
||||
require_finding "an empty name is reported" \
|
||||
"$(write_required empty-name "name: \"\"
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version: \"1.0.0\"")" \
|
||||
"name field is missing or empty"
|
||||
|
||||
echo ""
|
||||
echo "--- a file whose frontmatter block cannot be read reports THAT, not a missing field ---"
|
||||
# The required-field checks run downstream of the frontmatter match, so a file
|
||||
# with no readable block must land on the parse error rather than being reported
|
||||
# as a skill that merely forgot its version — and must never report green.
|
||||
require_finding "an unterminated frontmatter block is a parse error" \
|
||||
"$(write_required_raw unterminated "---
|
||||
name: unterminated
|
||||
description: $REQ_DESC
|
||||
metadata:
|
||||
version: \"1.0.0\"
|
||||
|
||||
Body text.
|
||||
")" \
|
||||
"no parseable YAML frontmatter block"
|
||||
require_finding "an empty '---/---' block is a parse error" \
|
||||
"$(write_required_raw empty-block "---
|
||||
---
|
||||
|
||||
Body text.
|
||||
")" \
|
||||
"no parseable YAML frontmatter block"
|
||||
|
||||
echo ""
|
||||
echo "Results: $PASS passed, $FAIL failed"
|
||||
[[ $FAIL -eq 0 ]]
|
||||
|
||||
Reference in New Issue
Block a user