fix(gates): close the review findings in check-skill-version-bump
- Read changed paths NUL-delimited so non-ASCII paths are no longer silently skipped. - Fail closed when only local main resolves and the pushed commit is the merge-base, instead of passing on an empty diff. - Accept ASCII-only versions with at most nine digits per part. - Check for python3/PyYAML up front, and report read failures as such rather than as a missing version; name a missing SKILL.md. - Document that pre-commit gates only the first ref of a multi-ref push. Tests grow to 29 cases covering each fix plus annotated tags, CRLF frontmatter, unrelated histories and pushing main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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" <<EOF
|
||||
#!/bin/sh
|
||||
PYTHONPATH="$SHIM_ROOT/shadow\${PYTHONPATH:+:\$PYTHONPATH}" exec "$REAL_PYTHON" "\$@"
|
||||
EOF
|
||||
# Passes the up-front import probe, crashes on the real read.
|
||||
cat > "$SHIM_ROOT/crash/python3" <<EOF
|
||||
#!/bin/sh
|
||||
[ "\$1" = "-c" ] && [ "\$2" = "import yaml" ] && exec "$REAL_PYTHON" "\$@"
|
||||
echo "Traceback: simulated interpreter failure" >&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 <repo> <skill>: 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 ]]
|
||||
|
||||
Reference in New Issue
Block a user