diff --git a/scripts/check-skill-version-bump.sh b/scripts/check-skill-version-bump.sh index ff9c0c7..5a6d230 100755 --- a/scripts/check-skill-version-bump.sh +++ b/scripts/check-skill-version-bump.sh @@ -6,42 +6,74 @@ set -euo pipefail # is the gate that holds the rule, since skill-size-check only checks presence # and shape. # -# Baseline: `git merge-base
`. 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. Unlike check-release-needed this runs on EVERY push, not -# only pushes to main: a missing bump is cheapest to fix on the branch, before -# review, and a push to main is then already covered. +# 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. +# +# Pushing main itself: with origin/main as the baseline, a push of main diffs +# the new commits against what the remote already has, so it is covered. A +# pushed commit that is already an ancestor of origin/main (merge-base equals +# the pushed commit) changes nothing relative to main and passes. With only the +# local `main` fallback, pushing main makes the merge-base the pushed commit +# itself — the diff is empty by construction, not because nothing changed — so +# that combination FAILS closed rather than passing unchecked. # # Scope: every skill directory plugins//.apm/skills//, bin # included. Anything under /tests/ is ignored — no agent ever loads it, # so a test-only change ships nothing to a reader. A skill counts as changed # when any other file under its directory differs between baseline and pushed -# ref. Renames are diffed as delete + add (--no-renames), so: +# commit. Paths are read NUL-delimited (`git diff -z`), so core.quotePath never +# hides a non-ASCII path. Renames are diffed as delete + add (--no-renames), so: # - a skill absent at the baseline (new, renamed-to, merged-into) is exempt; # it has no prior version to exceed. -# - a skill absent at the pushed ref (deleted, renamed-from) is exempt; -# there is nothing left to version. +# - a skill directory absent at the pushed commit (deleted, renamed-from) is +# exempt; there is nothing left to version. A directory that survives +# without its SKILL.md is NOT exempt: it fails as "SKILL.md missing". # A changed skill present at both refs must carry a three-part semver -# `metadata.version` at the pushed ref (same SEMVER_RE as skill-size-check.sh, -# so pre-release suffixes are rejected here as they are there) that is -# numerically greater than the baseline's. A baseline with no parseable -# version (a skill predating ADR-0022) accepts any valid version. Versions are -# read from git objects, never the working tree. +# `metadata.version` at the pushed commit that is numerically greater than the +# baseline's. The shape rule follows skill-size-check.sh (str()-coerce, strip +# whitespace and quotes, three dot-separated numbers, so `1.0` and `1.0.0-rc1` +# are rejected) but is deliberately stricter: ASCII digits only (Python's `\d` +# also matches e.g. U+FF11) and at most 9 digits per part, so every part fits +# bash arithmetic. A baseline with no valid version (a skill predating +# ADR-0022) accepts any valid version. Versions are read from git objects, +# never the working tree. # -# Missing baseline: if neither origin/main nor main resolves, or no merge-base -# exists (shallow clone, unrelated history), the gate FAILS closed — the same -# choice check-release-needed makes for an unreadable tag. Passing would make -# a fresh clone without main the one place the rule is silently off. +# Fails closed: if neither origin/main nor main resolves, if no merge-base +# exists (shallow clone, unrelated history), if the pushed ref does not resolve +# to a commit, if python3 or PyYAML is unavailable, or if a SKILL.md cannot be +# read. Passing in any of those would make that environment the one place the +# rule is silently off. # -# Known gap: this only fires on a local `git push` through pre-commit's -# pre-push hook (or a manual `pre-commit run --hook-stage pre-push`). A PR -# merged via Gitea's merge button runs no local hook at all — closing that -# requires a server-side CI job, which this repo does not have yet. +# Known gaps: +# - Only the first pushed ref is gated. pre-commit (4.x, hook_impl.py +# `_pre_push_ns`) consumes the pre-push stdin itself and builds the +# environment from the first ref line that is not a delete and has commits +# the remote lacks; every later ref in the same `git push` (e.g. +# `git push origin a b`, `--all`, `--tags`) is never seen. When that first +# ref's unpushed history reaches a root commit, pre-commit runs with +# all_files and sets no PRE_COMMIT_TO_REF at all, so this script checks +# HEAD — which is the pushed ref only if it happens to be checked out. The +# script cannot recover either case: the ref list is gone by the time it +# runs. Push refs one at a time to be sure each is checked. +# - A PR merged via Gitea's merge button runs no local hook at all (the same +# gap check-release-needed has). Closing it requires a server-side CI job, +# which this repo does not have yet. -# See check-release-needed.sh: PRE_COMMIT_TO_REF is the local sha actually -# being pushed, which is only HEAD for the common case. +# Byte-wise regex matching and messages: path bytes are matched against +# SKILL_PATH_RE below and must not depend on the caller's locale. +export LC_ALL=C + +# PRE_COMMIT_TO_REF is the local object actually being pushed, which is only +# HEAD for the common case. For a tag push it is the tag object, so it is +# peeled to a commit below before use. PUSHED_REF="${PRE_COMMIT_TO_REF:-HEAD}" # All-zeros sha: the push deletes a branch. Nothing ships; bail out. @@ -52,6 +84,23 @@ fi REPO_ROOT="$(git rev-parse --show-toplevel)" cd "$REPO_ROOT" +if ! command -v python3 > /dev/null 2>&1; then + echo "FAIL: python3 is required to read SKILL.md metadata.version but was not found on PATH." >&2 + echo " Fix: install python3 (pre-commit itself is a Python application, so it is almost certainly already present)." >&2 + exit 1 +fi + +if ! python3 -c 'import yaml' > /dev/null 2>&1; then + echo "FAIL: PyYAML is required to read SKILL.md metadata.version but is not importable by python3." >&2 + echo " Fix: python3 -m pip install PyYAML (or your distro's python3-yaml package)." >&2 + exit 1 +fi + +if ! PUSHED_COMMIT="$(git rev-parse --verify -q "$PUSHED_REF^{commit}")"; then + echo "FAIL: pushed ref $PUSHED_REF does not resolve to a commit." >&2 + exit 1 +fi + MAIN_REF="" for candidate in origin/main main; do if git rev-parse --verify -q "$candidate^{commit}" > /dev/null; then @@ -66,20 +115,29 @@ if [[ -z "$MAIN_REF" ]]; then exit 1 fi -if ! BASELINE="$(git merge-base "$MAIN_REF" "$PUSHED_REF" 2>/dev/null)"; then +if ! BASELINE="$(git merge-base "$MAIN_REF" "$PUSHED_COMMIT" 2>/dev/null)"; 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 ! CHANGED="$(git diff --no-renames --name-only "$BASELINE" "$PUSHED_REF" -- plugins)"; then +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 +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 path; do +while IFS= read -r -d '' path; do [[ "$path" =~ $SKILL_PATH_RE ]] || continue [[ "${BASH_REMATCH[2]}" == tests/* ]] && continue dir="${BASH_REMATCH[1]}" @@ -88,37 +146,54 @@ while IFS= read -r path; do [[ "$existing" == "$dir" ]] && { seen=true; break; } done $seen || SKILL_DIRS+=("$dir") -done <<< "$CHANGED" +done < "$CHANGED_FILE" [[ ${#SKILL_DIRS[@]} -eq 0 ]] && exit 0 -# Prints the three-part semver metadata.version from SKILL.md on stdin, or -# nothing when it is missing or malformed. Same acceptance rule as -# skill-size-check.sh: str()-coerce, strip whitespace and quotes, then -# ^\d+\.\d+\.\d+$ — so `1.0` (a YAML float) and `1.0.0-rc1` both print nothing. +# Reads SKILL.md bytes on stdin and prints exactly one line: `OK ` +# when metadata.version is a valid three-part semver, `INVALID` when it is +# missing or malformed (including unparseable frontmatter). Any other outcome — +# python3 crashing, PyYAML failing to import — is a non-zero exit with no OK / +# INVALID line, which the caller reports as a read failure, never as a missing +# version. Bytes are decoded explicitly so the caller's locale cannot turn a +# non-ASCII SKILL.md into a crash; `\s*` before each `\n` absorbs CRLF. read_version() { python3 -c ' import re, sys, yaml -text = sys.stdin.read() -m = re.match(r"^---\s*\n(.*?)\n---\s*(\n|$)", text, re.S) -if not m: - sys.exit(0) -try: - data = yaml.safe_load(m.group(1)) -except Exception: - sys.exit(0) +text = sys.stdin.buffer.read().decode("utf-8", errors="replace") +m = re.match(r"---[ \t\r]*\n(.*?)\n---[ \t\r]*(\n|\Z)", text, re.S) +data = None +if m: + try: + data = yaml.safe_load(m.group(1)) + except yaml.YAMLError: + data = None meta = data.get("metadata") if isinstance(data, dict) else None ver = meta.get("version") if isinstance(meta, dict) else None -if ver is None: - sys.exit(0) -ver = str(ver).strip().strip("\x27\"") -if re.match(r"^\d+\.\d+\.\d+$", ver): - print(ver) +ver = None if ver is None else str(ver).strip().strip("\x27\"") +if ver is not None and re.fullmatch(r"[0-9]{1,9}\.[0-9]{1,9}\.[0-9]{1,9}", ver): + print("OK " + ver) +else: + print("INVALID") ' } -# Exit 0 when $1 > $2, both MAJOR.MINOR.PATCH, compared numerically so -# 1.0.10 > 1.0.9. 10# forces base 10 on a leading zero. +# version_at : sets VERSION to the valid version or "" when +# invalid. Exits the script on a read failure. +version_at() { + local out + if ! out="$(git show "$1:$2" | read_version)" || [[ "$out" != OK\ * && "$out" != INVALID ]]; then + echo "FAIL: could not read metadata.version from $1:$2 (see error above)." >&2 + exit 1 + fi + VERSION="" + [[ "$out" == OK\ * ]] && VERSION="${out#OK }" + return 0 +} + +# Exit 0 when $1 > $2, both MAJOR.MINOR.PATCH with parts of at most 9 ASCII +# digits, compared numerically so 1.0.10 > 1.0.9. 10# forces base 10 on a +# leading zero. semver_gt() { local -a a b local i @@ -135,11 +210,18 @@ OFFENDERS=() for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do # Absent at baseline: new, renamed-to, or merged-into. Exempt. git cat-file -e "$BASELINE:$dir/SKILL.md" 2>/dev/null || continue - # Absent at pushed ref: deleted or renamed-from. Exempt. - [[ "$(git cat-file -t "$PUSHED_REF:$dir" 2>/dev/null)" == "tree" ]] || 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="$(git show "$BASELINE:$dir/SKILL.md" | read_version)" - cur_ver="$(git show "$PUSHED_REF:$dir/SKILL.md" 2>/dev/null | read_version || true)" + version_at "$BASELINE" "$dir/SKILL.md" + base_ver="$VERSION" + + if ! git cat-file -e "$PUSHED_COMMIT:$dir/SKILL.md" 2>/dev/null; then + OFFENDERS+=("$dir: SKILL.md missing at $PUSHED_REF (baseline: ${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})") diff --git a/tests/test-skill-version-bump.sh b/tests/test-skill-version-bump.sh index cc15b8b..1253a21 100755 --- a/tests/test-skill-version-bump.sh +++ b/tests/test-skill-version-bump.sh @@ -188,9 +188,11 @@ F="$(make_fixture)" write_skill "$F" demo alpha 'version: "1.0.0"' "unbumped"; commit "$F" BAD="$(cd "$F" && git rev-parse HEAD)" (cd "$F" && git checkout -q -b clean main) +echo "unrelated" > "$F/README.md"; commit "$F" +CLEAN="$(cd "$F" && git rev-parse HEAD)" expect_fail "unbumped TO_REF fails while HEAD is clean" "alpha: 1\.0\.0 -> 1\.0\.0" "$F" "$BAD" (cd "$F" && git checkout -q "$BAD") -expect_pass "clean TO_REF passes while HEAD is unbumped" "$F" "$(cd "$F" && git rev-parse main)" +expect_pass "clean TO_REF passes while HEAD is unbumped" "$F" "$CLEAN" echo "" echo "--- 14. all-zeros delete sha no-ops ---" @@ -247,6 +249,162 @@ F="$(mktemp -d)"; CLEANUP_DIRS+=("$F") write_skill "$F" demo alpha 'version: "1.0.0"'; commit "$F" expect_fail "missing main fails with a clear message" "neither origin/main nor main resolves" "$F" +echo "" +echo "--- 20. non-ASCII paths are not hidden by core.quotePath ---" +F="$(make_fixture)" +mkdir -p "$F/plugins/demo/.apm/skills/alpha/references" +echo "ref" > "$F/plugins/demo/.apm/skills/alpha/references/résumé.md"; commit "$F" +expect_fail "non-ASCII file under references/ without bump fails" "alpha: 1\.0\.0 -> 1\.0\.0" "$F" +F="$(make_fixture)" +(cd "$F" && git checkout -q main) +write_skill "$F" demo "café" 'version: "2.0.0"'; commit "$F" "add café" +(cd "$F" && git checkout -q feature && git merge -q main) +write_skill "$F" demo "café" 'version: "2.0.0"' "changed"; commit "$F" +expect_fail "non-ASCII skill dir without bump fails" "skills/café: 2\.0\.0 -> 2\.0\.0" "$F" +write_skill "$F" demo "café" 'version: "2.0.1"' "changed again"; commit "$F" +expect_pass "non-ASCII skill dir with bump passes" "$F" + +echo "" +echo "--- 21. local main fallback: pushed commit already in main fails closed ---" +F="$(make_fixture)" +MAIN_SHA="$(cd "$F" && git rev-parse main)" +expect_fail "pushing main's sha without origin/main fails" \ + "origin/main does not resolve.*already contained in local main" "$F" "$MAIN_SHA" +(cd "$F" && git checkout -q main) +expect_fail "HEAD on main without origin/main fails" "Fix: git fetch origin main" "$F" + +echo "" +echo "--- 22. pushing main itself with origin/main present ---" +F="$(make_fixture)" +(cd "$F" && git checkout -q main && git update-ref refs/remotes/origin/main main) +expect_pass "main equal to origin/main passes (nothing changed vs main)" "$F" +write_skill "$F" demo alpha 'version: "1.0.1"' "x"; commit "$F" +expect_pass "main ahead of origin/main with a bump passes" "$F" +write_skill "$F" demo beta "version: 1.0.9" "x"; commit "$F" +expect_fail "main ahead of origin/main without a bump fails" "beta: 1\.0\.9 -> 1\.0\.9" "$F" +(cd "$F" && git update-ref refs/remotes/origin/main main) +expect_pass "already-merged content (merge-base == pushed) passes against origin/main" "$F" \ + "$(cd "$F" && git rev-parse main~1)" + +echo "" +echo "--- 23. version shape is ASCII-only and bounded ---" +for v in 'version: "１.0.1"' 'version: "1.0.１"' 'version: "1.0.9999999999"' \ + 'version: "99999999999999999999.0.0"'; do + F="$(make_fixture)" + write_skill "$F" demo alpha "$v" "new body"; commit "$F" + OUT="$(run_check "$F" || true)" + if grep -q "alpha: metadata\.version missing or not" <<< "$OUT" \ + && ! grep -qiE "integer|syntax error|value too great" <<< "$OUT"; then + pass "'$v' is rejected as invalid without a bash arithmetic error" + else + fail "'$v' not cleanly rejected: $OUT" + fi +done +F="$(make_fixture)" +write_skill "$F" demo alpha 'version: "1.0.999999999"' "new body"; commit "$F" +expect_pass "nine-digit part is accepted and compared" "$F" + +echo "" +echo "--- 24. python3 / PyYAML failures are never reported as a missing version ---" +REAL_PYTHON="$(command -v python3)" +SHIM_ROOT="$(mktemp -d)"; CLEANUP_DIRS+=("$SHIM_ROOT") +mkdir -p "$SHIM_ROOT/shadow" "$SHIM_ROOT/noyaml" "$SHIM_ROOT/crash" +printf 'raise ImportError("PyYAML deliberately unavailable in this fixture")\n' \ + > "$SHIM_ROOT/shadow/yaml.py" +cat > "$SHIM_ROOT/noyaml/python3" < "$SHIM_ROOT/crash/python3" <&2 +exit 1 +EOF +chmod +x "$SHIM_ROOT/noyaml/python3" "$SHIM_ROOT/crash/python3" +if PATH="$SHIM_ROOT/noyaml:$PATH" python3 -c 'import yaml' 2>/dev/null; then + fail "fixture check: the no-PyYAML shim still imports yaml — the next assertion would be vacuous" +else + pass "fixture check: the no-PyYAML shim makes 'import yaml' fail" +fi +F="$(make_fixture)" +write_skill "$F" demo alpha 'version: "1.0.1"' "new body"; commit "$F" +OUT="$(cd "$F" && unset PRE_COMMIT_FROM_REF PRE_COMMIT_TO_REF PRE_COMMIT_REMOTE_BRANCH \ + && PATH="$SHIM_ROOT/noyaml:$PATH" bash "$SCRIPT" 2>&1)" && RC=0 || RC=$? +if [[ $RC -ne 0 ]] && grep -q "PyYAML is required" <<< "$OUT" && grep -q "Fix: python3 -m pip install PyYAML" <<< "$OUT" \ + && ! grep -q "Traceback" <<< "$OUT"; then + pass "missing PyYAML fails with FAIL + Fix, no traceback" +else + fail "missing PyYAML not reported cleanly (rc=$RC): $OUT" +fi +OUT="$(cd "$F" && unset PRE_COMMIT_FROM_REF PRE_COMMIT_TO_REF PRE_COMMIT_REMOTE_BRANCH \ + && PATH="$SHIM_ROOT/crash:$PATH" bash "$SCRIPT" 2>&1)" && RC=0 || RC=$? +if [[ $RC -ne 0 ]] && grep -q "could not read metadata.version" <<< "$OUT" \ + && ! grep -q "missing or not" <<< "$OUT"; then + pass "python3 crash during the read is a read failure, not a missing version" +else + fail "python3 crash misreported (rc=$RC): $OUT" +fi + +echo "" +echo "--- 25. SKILL.md deleted but skill dir kept ---" +F="$(make_fixture)" +mkdir -p "$F/plugins/demo/.apm/skills/alpha/references" +echo "ref" > "$F/plugins/demo/.apm/skills/alpha/references/x.md" +rm "$F/plugins/demo/.apm/skills/alpha/SKILL.md"; commit "$F" +expect_fail "missing SKILL.md is reported as such" "alpha: SKILL\.md missing at HEAD \(baseline: 1\.0\.0\)" "$F" + +echo "" +echo "--- 26. baseline without a valid version accepts any valid version ---" +F="$(make_fixture)" +(cd "$F" && git checkout -q main) +write_skill "$F" demo legacy ""; commit "$F" "add legacy skill" +(cd "$F" && git checkout -q feature && git merge -q main) +write_skill "$F" demo legacy 'version: "0.0.1"' "changed"; commit "$F" +expect_pass "invalid baseline + valid current passes" "$F" + +echo "" +echo "--- 27. no merge-base (unrelated histories) fails closed ---" +F="$(make_fixture)" +(cd "$F" && git checkout -q --orphan unrelated && git rm -rq --cached . && rm -rf plugins) +write_skill "$F" demo alpha 'version: "1.0.0"' "orphan"; commit "$F" "orphan root" +expect_fail "unrelated history fails at the merge-base check" "no merge-base between main and HEAD" "$F" + +echo "" +echo "--- 28. annotated tag push is peeled to its commit ---" +F="$(make_fixture)" +write_skill "$F" demo alpha 'version: "1.0.0"' "unbumped"; commit "$F" +(cd "$F" && git tag -a v9 -m "tag" && git checkout -q main) +TAG_OBJ="$(cd "$F" && git rev-parse v9)" +if [[ "$(cd "$F" && git cat-file -t "$TAG_OBJ")" == "tag" ]]; then + pass "fixture check: TO_REF is a tag object, not a commit" +else + fail "fixture check: v9 is not an annotated tag object" +fi +expect_fail "unbumped change behind an annotated tag fails" "alpha: 1\.0\.0 -> 1\.0\.0" "$F" "$TAG_OBJ" + +echo "" +echo "--- 29. CRLF frontmatter is parsed ---" +# crlf : rewrite that skill's SKILL.md with CRLF line endings. +crlf() { + local f="$1/plugins/demo/.apm/skills/$2/SKILL.md" + sed 's/$/\r/' "$f" > "$f.tmp" && mv "$f.tmp" "$f" +} +F="$(make_fixture)" +(cd "$F" && git config core.autocrlf false && git checkout -q main) +crlf "$F" alpha; commit "$F" "alpha to CRLF" +(cd "$F" && git checkout -q feature && git merge -q main) +write_skill "$F" demo alpha 'version: "1.0.0"' "crlf body"; crlf "$F" alpha; commit "$F" +if grep -q $'\r' "$F/plugins/demo/.apm/skills/alpha/SKILL.md"; then + pass "fixture check: SKILL.md carries CRLF" +else + fail "fixture check: SKILL.md has no CRLF" +fi +expect_fail "unbumped CRLF skill reports both parsed versions" "alpha: 1\.0\.0 -> 1\.0\.0" "$F" +write_skill "$F" demo alpha 'version: "1.0.1"' "crlf body 2"; crlf "$F" alpha; commit "$F" +expect_pass "bumped CRLF skill passes" "$F" + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]]