From ea119d83b0768365551ad722d8f4f805ed68b177 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Sat, 19 Sep 2026 21:08:10 +0000 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01NwD8Egs5r4ndqeFLmhusX2 --- .pre-commit-config.yaml | 2 +- .../.apm/hooks/check-apm-current.sh | 17 +- scripts/check-skill-version-bump.sh | 160 +++++++++---- tests/run-tests.sh | 61 +++++ tests/test-adr0020-frontmatter.sh | 225 ++++++++++++++++++ tests/test-apm-current-hook.sh | 37 +++ tests/test-run-tests.sh | 68 ++++++ tests/test-skill-version-bump.sh | 45 ++++ tests/test-vale-wrap.sh | 127 +++++++++- 9 files changed, 693 insertions(+), 49 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4d784e5..395d3c5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -215,7 +215,7 @@ repos: - id: skill-size-check stages: ['pre-commit'] name: SKILL.md size and context-budget ceilings - description: Enforce agentskills.io's 500-line/2,770-whole-file-word spec ceilings AND ADR-0020's context budget -- description 250 chars SUGGESTION / 400 FAIL, body-only 600 words SUGGESTION / 900 FAIL, and every boundary-clause routing target resolving to a real skill or agent under plugins/*/.apm/ + description: Enforce agentskills.io's 500-line/2,770-whole-file-word spec ceilings AND ADR-0020's context budget -- description 250 chars SUGGESTION / 400 FAIL, body-only 600 words SUGGESTION / 900 FAIL, and every boundary-clause routing target resolving to a real skill or agent under plugins/*/.apm/ -- plus the required frontmatter fields folded in from the former skill-frontmatter hook, namely name, a non-empty description, and a metadata.version matching three-part semver (1.0.0) entry: scripts/skill-size-check.sh language: script files: '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$' diff --git a/plugins/kyberforge/.apm/hooks/check-apm-current.sh b/plugins/kyberforge/.apm/hooks/check-apm-current.sh index 1475303..2f2b80e 100755 --- a/plugins/kyberforge/.apm/hooks/check-apm-current.sh +++ b/plugins/kyberforge/.apm/hooks/check-apm-current.sh @@ -56,12 +56,21 @@ emit() { # is churn unrelated to the branch and should be discarded. The branch name only # selects between fixed strings and is never interpolated. Outside a git checkout, # or on a detached HEAD, the neutral advice stands. +# +# So does an UNSET origin/HEAD, which is the common state: git only writes it on +# clone, and `git remote add` never does. The fallback here used to be `main`, +# which is a guess, and it is wrong in exactly the repos that would notice — a +# checkout whose default branch is `master` was told "this is a feature branch, +# so discard it" while standing on its default branch, i.e. told to throw away a +# real lock update. There is no cheap way to learn the remote's default without +# the network, so nothing is asserted: the advice stays neutral and the reader +# decides. lock_advice="commit it or discard it deliberately." current_branch="$(git symbolic-ref --short -q HEAD 2> /dev/null || true)" -if [[ -n "$current_branch" ]]; then - default_branch="$(git symbolic-ref --short -q refs/remotes/origin/HEAD 2> /dev/null || true)" - default_branch="${default_branch#origin/}" - if [[ "$current_branch" == "${default_branch:-main}" ]]; then +default_branch="$(git symbolic-ref --short -q refs/remotes/origin/HEAD 2> /dev/null || true)" +default_branch="${default_branch#origin/}" +if [[ -n "$current_branch" && -n "$default_branch" ]]; then + if [[ "$current_branch" == "$default_branch" ]]; then lock_advice="this is the default branch, so commit it or discard it deliberately." else lock_advice="this is a feature branch, so discard it: git checkout -- apm.lock.yaml && apm install" diff --git a/scripts/check-skill-version-bump.sh b/scripts/check-skill-version-bump.sh index 8490427..36afbf7 100755 --- a/scripts/check-skill-version-bump.sh +++ b/scripts/check-skill-version-bump.sh @@ -6,16 +6,29 @@ set -euo pipefail # is the gate that holds the rule, since skill-size-check only checks presence # and shape. # -# Baseline: `git merge-base
`, where
is origin/main -# when it resolves and the local `main` branch otherwise. Readers install -# skills from main, so "changed" means changed relative to what main ships, not -# relative to the remote branch's current tip. Diffing from PRE_COMMIT_FROM_REF -# would let the second push of a feature branch excuse a change the first push -# already carried unbumped. The check runs on every push whatever the target -# branch — nothing here reads PRE_COMMIT_REMOTE_BRANCH — so it also runs under -# a manual `pre-commit run --hook-stage pre-push` (against HEAD, since no -# PRE_COMMIT_TO_REF is set). A missing bump is cheapest to fix on the branch, -# before review. +# Baseline: `git merge-base --all
`, where
is +# origin/main when it resolves and the local `main` branch otherwise. Readers +# install skills from main, so "changed" means changed relative to what main +# ships, not relative to the remote branch's current tip. Diffing from +# PRE_COMMIT_FROM_REF would let the second push of a feature branch excuse a +# change the first push already carried unbumped. The check runs on every push +# whatever the target branch — nothing here reads PRE_COMMIT_REMOTE_BRANCH — so +# it also runs under a manual `pre-commit run --hook-stage pre-push` (against +# HEAD, since no PRE_COMMIT_TO_REF is set). A missing bump is cheapest to fix on +# the branch, before review. +# +# `--all`, not the single base git would otherwise pick for it. A criss-cross +# history — main merges a branch while that branch merges a main commit — has +# TWO merge bases, and which one `git merge-base` prints is an implementation +# detail. Picking one made the verdict a coin flip: a skill byte-identical to +# main's tip was still reported "not above merge-base" whenever the losing base +# happened to be chosen, so an already-merged bump failed the push it should +# have passed. So a skill counts as CHANGED only when it differs from EVERY +# base — differing from none of them, or from only some, means one base already +# carries the pushed content — and a changed skill's version must exceed the +# version at every base it exists at. Both directions are conservative: the +# intersection cannot exempt a skill that genuinely changed since all of main's +# reachable history, and requiring every base keeps the ratchet. # # Second baseline: the tip of that same
ref. A changed skill's pushed # version must exceed its version there too (ADR-0022, second 2026-09-16 @@ -134,38 +147,70 @@ if [[ -z "$MAIN_REF" ]]; then exit 1 fi -if ! BASELINE="$(git merge-base "$MAIN_REF" "$PUSHED_COMMIT" 2>/dev/null)"; then +BASES=() +while IFS= read -r base; do + [[ -n "$base" ]] && BASES+=("$base") +done < <(git merge-base --all "$MAIN_REF" "$PUSHED_COMMIT" 2>/dev/null || true) + +if [[ ${#BASES[@]} -eq 0 ]]; then echo "FAIL: no merge-base between $MAIN_REF and $PUSHED_REF, so there is no baseline to compare skill versions against." >&2 echo " Fix: ensure full history is available (e.g. git fetch --unshallow) and retry." >&2 exit 1 fi -if [[ "$MAIN_REF" == "main" && "$BASELINE" == "$PUSHED_COMMIT" ]]; then - echo "FAIL: origin/main does not resolve and $PUSHED_REF is already contained in local main, so local main cannot serve as an independent baseline — the diff would be empty by construction." >&2 - echo " Fix: git fetch origin main and retry." >&2 - exit 1 +# A pushed commit that is an ancestor of
is itself the only merge-base, +# so this fires on exactly the case it always did. +if [[ "$MAIN_REF" == "main" ]]; then + for base in ${BASES[@]+"${BASES[@]}"}; do + if [[ "$base" == "$PUSHED_COMMIT" ]]; then + echo "FAIL: origin/main does not resolve and $PUSHED_REF is already contained in local main, so local main cannot serve as an independent baseline — the diff would be empty by construction." >&2 + echo " Fix: git fetch origin main and retry." >&2 + exit 1 + fi + done fi CHANGED_FILE="$(mktemp)" trap 'rm -f "$CHANGED_FILE"' EXIT -if ! git diff -z --no-renames --name-only "$BASELINE" "$PUSHED_COMMIT" -- plugins > "$CHANGED_FILE"; then - echo "FAIL: could not diff $BASELINE..$PUSHED_REF (see git error above)." >&2 - exit 1 -fi - SKILL_PATH_RE='^(plugins/[^/]+/\.apm/skills/[^/]+)/(.+)$' -SKILL_DIRS=() -while IFS= read -r -d '' path; do - [[ "$path" =~ $SKILL_PATH_RE ]] || continue - [[ "${BASH_REMATCH[2]}" == tests/* ]] && continue - dir="${BASH_REMATCH[1]}" - seen=false - for existing in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do - [[ "$existing" == "$dir" ]] && { seen=true; break; } + +# skill_dirs_at : sets SKILL_DIRS_ONE to the skill directories differing +# between and the pushed commit. +skill_dirs_at() { + local path dir existing seen + SKILL_DIRS_ONE=() + if ! git diff -z --no-renames --name-only "$1" "$PUSHED_COMMIT" -- plugins > "$CHANGED_FILE"; then + echo "FAIL: could not diff $1..$PUSHED_REF (see git error above)." >&2 + exit 1 + fi + while IFS= read -r -d '' path; do + [[ "$path" =~ $SKILL_PATH_RE ]] || continue + [[ "${BASH_REMATCH[2]}" == tests/* ]] && continue + dir="${BASH_REMATCH[1]}" + seen=false + for existing in ${SKILL_DIRS_ONE[@]+"${SKILL_DIRS_ONE[@]}"}; do + [[ "$existing" == "$dir" ]] && { seen=true; break; } + done + $seen || SKILL_DIRS_ONE+=("$dir") + done < "$CHANGED_FILE" +} + +# Intersected across every base: a skill matching any ONE base is already +# shipped by that base and has nothing left to bump. +skill_dirs_at "${BASES[0]}" +SKILL_DIRS=(${SKILL_DIRS_ONE[@]+"${SKILL_DIRS_ONE[@]}"}) +for ((i = 1; i < ${#BASES[@]}; i++)); do + [[ ${#SKILL_DIRS[@]} -eq 0 ]] && break + skill_dirs_at "${BASES[i]}" + KEPT=() + for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do + for existing in ${SKILL_DIRS_ONE[@]+"${SKILL_DIRS_ONE[@]}"}; do + [[ "$existing" == "$dir" ]] && { KEPT+=("$dir"); break; } + done done - $seen || SKILL_DIRS+=("$dir") -done < "$CHANGED_FILE" + SKILL_DIRS=(${KEPT[@]+"${KEPT[@]}"}) +done [[ ${#SKILL_DIRS[@]} -eq 0 ]] && exit 0 @@ -238,35 +283,64 @@ in_tree() { OFFENDERS=() for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do - at_base=false + # Bases the skill exists at, in merge-base order, with the version read at + # each. Index-matched arrays rather than one map: bash 3.2 has no `declare -A`. + base_at=() + base_vers=() + first_base_ver="" + for base in ${BASES[@]+"${BASES[@]}"}; do + in_tree "$base" "$dir/SKILL.md" || continue + version_at "$base" "$dir/SKILL.md" + base_at+=("$base") + base_vers+=("$VERSION") + [[ -n "$first_base_ver" ]] || first_base_ver="$VERSION" + done + + # When main has not moved since a merge-base, the tip IS that baseline and the + # skill is checked once. at_tip=false - in_tree "$BASELINE" "$dir/SKILL.md" && at_base=true - # When main has not moved since the merge-base, the tip is the same baseline. - [[ "$MAIN_TIP" != "$BASELINE" ]] && in_tree "$MAIN_TIP" "$dir/SKILL.md" && at_tip=true - # Absent at both baselines: new, renamed-to, or merged-into. Exempt. - $at_base || $at_tip || continue + tip_is_base=false + for base in ${BASES[@]+"${BASES[@]}"}; do + if [[ "$MAIN_TIP" == "$base" ]]; then + tip_is_base=true + break + fi + done + if [[ "$tip_is_base" == false ]] && in_tree "$MAIN_TIP" "$dir/SKILL.md"; then + at_tip=true + fi + # Absent at every baseline: new, renamed-to, or merged-into. Exempt. + [[ ${#base_at[@]} -gt 0 ]] || $at_tip || continue # Directory absent at pushed commit: deleted or renamed-from. Exempt. [[ "$(git cat-file -t "$PUSHED_COMMIT:$dir" 2>/dev/null)" == "tree" ]] || continue - base_ver="" tip_ver="" - if $at_base; then version_at "$BASELINE" "$dir/SKILL.md"; base_ver="$VERSION"; fi if $at_tip; then version_at "$MAIN_TIP" "$dir/SKILL.md"; tip_ver="$VERSION"; fi if ! in_tree "$PUSHED_COMMIT" "$dir/SKILL.md"; then - OFFENDERS+=("$dir: SKILL.md missing at $PUSHED_REF (baseline: ${base_ver:-none})") + OFFENDERS+=("$dir: SKILL.md missing at $PUSHED_REF (baseline: ${first_base_ver:-none})") continue fi version_at "$PUSHED_COMMIT" "$dir/SKILL.md" cur_ver="$VERSION" if [[ -z "$cur_ver" ]]; then - OFFENDERS+=("$dir: metadata.version missing or not MAJOR.MINOR.PATCH at $PUSHED_REF (baseline: ${base_ver:-none})") + OFFENDERS+=("$dir: metadata.version missing or not MAJOR.MINOR.PATCH at $PUSHED_REF (baseline: ${first_base_ver:-none})") continue fi - if [[ -n "$base_ver" ]] && ! semver_gt "$cur_ver" "$base_ver"; then - OFFENDERS+=("$dir: $base_ver -> $cur_ver (not above merge-base)") - fi + # Named by sha only when there is more than one base to tell apart; a + # criss-cross history is the only case where "which merge-base" is a question + # the reader cannot answer from the branch alone. + for ((i = 0; i < ${#base_at[@]}; i++)); do + base_ver="${base_vers[i]}" + [[ -n "$base_ver" ]] || continue + semver_gt "$cur_ver" "$base_ver" && continue + if [[ ${#BASES[@]} -gt 1 ]]; then + OFFENDERS+=("$dir: $base_ver -> $cur_ver (not above merge-base ${base_at[i]})") + else + OFFENDERS+=("$dir: $base_ver -> $cur_ver (not above merge-base)") + fi + done if [[ -n "$tip_ver" ]] && ! semver_gt "$cur_ver" "$tip_ver"; then OFFENDERS+=("$dir: $tip_ver -> $cur_ver (not above $MAIN_REF tip)") fi diff --git a/tests/run-tests.sh b/tests/run-tests.sh index 1080bd2..bc86632 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -169,6 +169,67 @@ done < <( | sort ) +# Discovering NOTHING is never a clean run, and it used to be the quietest +# possible pass: the loops below iterate zero times, nothing is printed between +# the bats block and the summary, and `Summary: 0 passed, 0 failed` exits 0 -- +# under --strict too, because strictness only ever turned SKIPS into failures +# and there were no suites to skip. A wrong TEST_DIR, a mistyped `find` pattern, +# an exclusion that grew to swallow tests/, and a gutted checkout all land here. +# tests/run-bats.sh has carried this guard for its own .bats discovery; this is +# the same guard one file over, and the run-tests pre-push hook is the caller +# that needs it. +# +# Two checks, in the same order and for the same reasons as run-bats.sh's. +# First, the derived one: every test-*.sh in the git index must have been +# discovered. The direction matters -- a discovered file need NOT be tracked +# (work in progress is ordinary), and a file removed with `git rm` leaves the +# index, so a deliberate removal passes while an accidental disappearance +# fails. It only runs when SEARCH_ROOT is itself the git worktree root, which +# is what keeps it off the mktemp fixture trees in tests/test-run-tests.sh -- +# those hold one or two test-*.sh files by design and git resolves no worktree +# for them. The same exclusions are reapplied to the index listing so both +# sides cover the same universe. +EXPECTED_SCRIPTS=() +GIT_TOPLEVEL="$(git -C "$SEARCH_ROOT" rev-parse --show-toplevel 2> /dev/null || true)" +if [[ -n "$GIT_TOPLEVEL" && "$GIT_TOPLEVEL" == "$SEARCH_ROOT" ]]; then + while IFS= read -r f; do + [[ -n "$f" ]] && EXPECTED_SCRIPTS+=("$SEARCH_ROOT/$f") + done < <( + git -C "$SEARCH_ROOT" ls-files -- 'test-*.sh' '*/test-*.sh' \ + | grep -Ev '(^|/)\.claude/worktrees/|(^|/)apm_modules/|(^|/)\.claude/skills/' \ + | sort || true + ) +fi + +if [[ ${#EXPECTED_SCRIPTS[@]} -gt 0 ]]; then + MISSING_SCRIPTS=() + for expected in ${EXPECTED_SCRIPTS[@]+"${EXPECTED_SCRIPTS[@]}"}; do + found=false + for actual in ${SCRIPTS[@]+"${SCRIPTS[@]}"}; do + if [[ "$actual" == "$expected" ]]; then + found=true + break + fi + done + [[ "$found" == true ]] || MISSING_SCRIPTS+=("${expected#"$SEARCH_ROOT"/}") + done + if [[ ${#MISSING_SCRIPTS[@]} -gt 0 ]]; then + echo "Error: ${#MISSING_SCRIPTS[@]} of ${#EXPECTED_SCRIPTS[@]} tracked test-*.sh file(s) were not discovered under $SEARCH_ROOT — they were deleted without being removed from the index, or the search path/exclusions above no longer reach them:" >&2 + for m in ${MISSING_SCRIPTS[@]+"${MISSING_SCRIPTS[@]}"}; do + echo " $m" >&2 + done + exit 1 + fi +fi + +# Second, unconditional and separate: a tree with nothing tracked (a tarball +# export, a fresh scaffold) still must not run on an empty set and call it +# green. +if [[ ${#SCRIPTS[@]} -eq 0 ]]; then + echo "Error: found 0 test-*.sh file(s) under $SEARCH_ROOT — the search path is wrong or the suite has been gutted" >&2 + exit 1 +fi + # Each test-*.sh is independent (fixtures live under its own mktemp dir, none # write back into the live repo tree -- verified before adding this), so they # run concurrently in fixed-size batches instead of one at a time. Dispatch and diff --git a/tests/test-adr0020-frontmatter.sh b/tests/test-adr0020-frontmatter.sh index 4d06d26..af25bc1 100755 --- a/tests/test-adr0020-frontmatter.sh +++ b/tests/test-adr0020-frontmatter.sh @@ -441,6 +441,231 @@ else fail "validate.sh agent mode exited $SILENT_RC with output '${SILENT_OUT:-}' — 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 [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 — 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