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