fix(scripts): fail the vale and scope gates when they cannot verify
check-vale-style-sync.sh's glob-coverage probe silently self-disabled when vale was absent from PATH, exiting 0 on the one-character glob typo it exists to catch. pre-commit swallows a passing hook's output, so the pre-push hook reported Passed. The script already hard-fails on a bad REPO_ROOT for exactly this reason -- "a clean exit 0 here would read as 'checked, in sync' when nothing ran at all" -- and six of its assertions are vale invocations. Absence now fails; the opt-out is an env var that must be set deliberately, and it downgrades the run to text-level assertions while saying so in the summary. Neither script had a floor on its rewritten .apm/ paths, so relocating .apm/ made both exit 0 -- and this PR's whole change to them was a path rewrite, the exact edit that failure mode survives. A third gap the directory check could not see: relocating only assets/vale/ left both audit skill directories in place while every probe continued past its missing .vale.ini, skipping the whole table with FAIL=0. A zero-probe run is now an error. Both test suites encoded the vacuous pass as a passing case. Those cases are now scoped to "no plugins/kyberforge at all" and assert the fixture really lacks it, with new counterparts covering the drift shape and new positive cases requiring each script to report a non-zero inspected-target count. Also removes the HOOK_REGEX_CACHE memoization: every call site was a command substitution, so the writes happened in a subshell and the lookup always missed. Measured at 14ms of an ~870ms run, all of which is the six vale invocations. Deleted rather than repaired -- 35 lines claiming a benefit they never delivered is worse than no cache -- with a comment recording why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -9,6 +9,28 @@ FAIL=0
|
||||
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
||||
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
||||
|
||||
# Same `exit 77` (automake convention; run-tests.sh renders it as SKIPPED) guard
|
||||
# tests/test-vale-wrap.sh and tests/test-sync-plugin-content.sh use for a missing
|
||||
# binary. Without it this suite reported 5 genuine failures on a machine with no
|
||||
# vale, none of which were regressions.
|
||||
#
|
||||
# The suite SKIPPING while the script it tests HARD-FAILS is deliberate, not an
|
||||
# inconsistency. The script is a pre-push gate whose exit 0 is a claim that the
|
||||
# repo was verified, and six of its assertions are vale invocations — it must
|
||||
# never make that claim on a machine where they could not run. This suite makes
|
||||
# no claim about the repo; it claims the script behaves correctly, and most of
|
||||
# its cases (every glob-coverage case, 10/10b/11) cannot be exercised at all
|
||||
# without vale. Reporting those as FAIL would say "a regression landed" when the
|
||||
# truth is "this machine is missing a dev dependency" — noise that competes with
|
||||
# real failures. Note also that the vale-absent behavior is still fully covered
|
||||
# here even so: the masking below constructs that condition deliberately on a
|
||||
# machine that HAS vale, which is the only place it can be asserted against a
|
||||
# known-good baseline.
|
||||
if ! command -v vale &>/dev/null; then
|
||||
echo "SKIP: vale is not installed — the glob-coverage cases cannot run (install it: https://vale.sh/docs/vale-cli/installation/)"
|
||||
exit 77
|
||||
fi
|
||||
|
||||
# One trap over a registry, rather than rebuilding the trap line per fixture:
|
||||
# the guard is there because bash 3.2 treats "${arr[@]}" on an empty array as
|
||||
# unbound under `set -u`.
|
||||
@@ -89,9 +111,18 @@ fi
|
||||
# Runs the check with vale masked off PATH when that is safe. For text-only
|
||||
# assertions ONLY — never for a case whose verdict depends on a glob probe
|
||||
# actually running.
|
||||
#
|
||||
# CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 is required now that the script
|
||||
# treats a missing vale as a FAIL rather than a warning: without the opt-out
|
||||
# every masked run exits 1 unconditionally and every negative case below would
|
||||
# pass vacuously — the precise vacuity this whole round is closing. The opt-out
|
||||
# restores what masking is for here: the text assertion under test becomes the
|
||||
# only thing that can produce a non-zero exit. Case 12 asserts the un-opted-out
|
||||
# masked run really does hard-fail, so this env var cannot quietly become the
|
||||
# only path anyone exercises.
|
||||
run_check_no_vale() {
|
||||
if [[ "$VALE_MASKED" == true ]]; then
|
||||
PATH="$PATH_NO_VALE" bash "$SCRIPT" "$@"
|
||||
PATH="$PATH_NO_VALE" CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 bash "$SCRIPT" "$@"
|
||||
else
|
||||
bash "$SCRIPT" "$@"
|
||||
fi
|
||||
@@ -151,15 +182,93 @@ else
|
||||
pass "exits non-zero when a rule file is missing from one copy"
|
||||
fi
|
||||
|
||||
# --- 5. Exits 0 (no-op) when kyberforge isn't present in the target repo ---
|
||||
# --- 5. Exits 0 (no-op) ONLY when there is no kyberforge plugin at all ---
|
||||
# The no-op is scoped to a repo that never installed kyberforge. Case 5c below
|
||||
# is its counterpart and the one that matters: `plugins/kyberforge/` present but
|
||||
# the `.apm/` targets under it absent is drift, not absence.
|
||||
echo ""
|
||||
echo "--- exits 0 when kyberforge skills are absent (no-op) ---"
|
||||
echo "--- exits 0 when there is no plugins/kyberforge at all (no-op) ---"
|
||||
FIXTURE5="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE5")
|
||||
if bash "$SCRIPT" "$FIXTURE5" > /dev/null 2>&1; then
|
||||
pass "exits 0 as a no-op when skill-audit/agent-audit don't exist"
|
||||
if [[ -e "$FIXTURE5/plugins/kyberforge" ]]; then
|
||||
fail "fixture 5 unexpectedly has a plugins/kyberforge, so it does not exercise the no-kyberforge no-op"
|
||||
elif bash "$SCRIPT" "$FIXTURE5" > /dev/null 2>&1; then
|
||||
pass "exits 0 as a no-op when the repo has no kyberforge plugin"
|
||||
else
|
||||
fail "exited non-zero when skill-audit/agent-audit are simply absent"
|
||||
fail "exited non-zero when the repo simply has no kyberforge plugin"
|
||||
fi
|
||||
|
||||
# --- 5c. Exits 1, saying so, when plugins/kyberforge exists but its .apm/
|
||||
# targets do not ---
|
||||
# This script hardcodes plugins/kyberforge/.apm/skills/{skill-audit,agent-audit}
|
||||
# and had no floor under them: `mv plugins/kyberforge/.apm plugins/kyberforge/.apm2`
|
||||
# made both directories absent, which fell into the no-op above and exited 0 —
|
||||
# indistinguishable from a verified in-sync result, and swallowed by pre-commit
|
||||
# as `Passed`. A path rewrite is exactly the edit that produces this, and it is
|
||||
# what this PR did to these paths.
|
||||
#
|
||||
# Exit code alone proves little here (the script exits 1 for a dozen reasons), so
|
||||
# assert the MESSAGE: deleting the floor leaves exit 0, but a floor that fired
|
||||
# for the wrong reason would still be a bug this case must catch.
|
||||
echo ""
|
||||
echo "--- exits 1 and says so when plugins/kyberforge exists but .apm/ does not ---"
|
||||
FIXTURE5C="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE5C")
|
||||
mv "$FIXTURE5C/plugins/kyberforge/.apm" "$FIXTURE5C/plugins/kyberforge/.apm2"
|
||||
STALE_OUT=""
|
||||
STALE_RC=0
|
||||
STALE_OUT="$(bash "$SCRIPT" "$FIXTURE5C" 2>&1)" || STALE_RC=$?
|
||||
if [[ $STALE_RC -eq 0 ]]; then
|
||||
fail "exited 0 when plugins/kyberforge exists but its .apm/ targets are gone — expected exit 1"
|
||||
elif ! printf '%s\n' "$STALE_OUT" | grep -q "\.apm/ paths have gone stale"; then
|
||||
fail "failed for the wrong reason on a stale .apm/ path: $(printf '%s' "$STALE_OUT" | tr '\n' ' ')"
|
||||
else
|
||||
pass "exits non-zero and reports a stale .apm/ path when plugins/kyberforge exists without it"
|
||||
fi
|
||||
|
||||
# --- 5d. Exits 1, saying so, when the probe table verifies nothing ---
|
||||
# The directory floor above cannot see this one: both audit skill directories are
|
||||
# still in place, only `assets/vale/` has moved. Every probe then `continue`s on
|
||||
# its missing .vale.ini and the glob-coverage section checks zero paths. Asserted
|
||||
# on the message because several other assertions also fire on this fixture.
|
||||
echo ""
|
||||
echo "--- exits 1 and says so when zero glob probes were checked ---"
|
||||
FIXTURE5D="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE5D")
|
||||
mv "$FIXTURE5D/plugins/kyberforge/.apm/skills/skill-audit/assets/vale" \
|
||||
"$FIXTURE5D/plugins/kyberforge/.apm/skills/skill-audit/assets/vale-moved"
|
||||
mv "$FIXTURE5D/plugins/kyberforge/.apm/skills/agent-audit/assets/vale" \
|
||||
"$FIXTURE5D/plugins/kyberforge/.apm/skills/agent-audit/assets/vale-moved"
|
||||
NOPROBE_OUT=""
|
||||
NOPROBE_RC=0
|
||||
NOPROBE_OUT="$(bash "$SCRIPT" "$FIXTURE5D" 2>&1)" || NOPROBE_RC=$?
|
||||
if [[ $NOPROBE_RC -eq 0 ]]; then
|
||||
fail "exited 0 when no glob probe could be checked — expected exit 1"
|
||||
elif ! printf '%s\n' "$NOPROBE_OUT" | grep -q "no probe path was checked"; then
|
||||
fail "did not report that zero probe paths were checked: $(printf '%s' "$NOPROBE_OUT" | tr '\n' ' ')"
|
||||
else
|
||||
pass "exits non-zero and reports that zero glob probes were checked"
|
||||
fi
|
||||
|
||||
# --- 5e. Positive: the check does real work against THIS repo ---
|
||||
# Every case above runs against a synthetic fixture, so the whole suite could be
|
||||
# green while the script inspected nothing at all in the repo it is wired into at
|
||||
# pre-push. The summary line carries the counts; assert they are non-zero.
|
||||
echo ""
|
||||
echo "--- reports a non-zero number of inspected targets against this repo ---"
|
||||
REAL_OUT=""
|
||||
REAL_RC=0
|
||||
REAL_OUT="$(bash "$SCRIPT" "$REPO_ROOT" 2>&1)" || REAL_RC=$?
|
||||
REAL_PROBES="$(printf '%s\n' "$REAL_OUT" | sed -n 's/.*checked, \([0-9][0-9]*\) glob probe(s).*/\1/p')"
|
||||
if [[ $REAL_RC -ne 0 ]]; then
|
||||
fail "exited non-zero against this repo's real Vale copies"
|
||||
printf '%s\n' "$REAL_OUT" | sed 's/^/ /'
|
||||
elif [[ -z "$REAL_PROBES" ]]; then
|
||||
fail "a clean run against this repo reported no inspected-target counts, so 'it checked something' is unverifiable: $(printf '%s' "$REAL_OUT" | tr '\n' ' ')"
|
||||
elif [[ "$REAL_PROBES" -lt 1 ]]; then
|
||||
fail "a clean run against this repo verified $REAL_PROBES glob probes — a pass that inspected nothing"
|
||||
else
|
||||
pass "inspects $REAL_PROBES glob probe(s) against this repo, and exits 0"
|
||||
fi
|
||||
|
||||
# --- 5b. Exits 1 when REPO_ROOT does not exist ---
|
||||
@@ -527,26 +636,78 @@ else
|
||||
'StylesPath = styles' 'StylesPath = elsewhere'
|
||||
break_glob "$FIXTURE19/plugins/kyberforge/.apm/skills/agent-audit/assets/vale/.vale.ini" \
|
||||
'BasedOnStyles = Kyberforge' 'BasedOnStyles = KyberforgeCopilot'
|
||||
if PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE17" > /dev/null 2>&1; then
|
||||
pass "exits 0 on in-sync copies with vale unavailable"
|
||||
# 12a. Missing vale is a HARD FAILURE, not a warning — even on copies that are
|
||||
# otherwise perfectly in sync. It used to be a warning, and a warning made the
|
||||
# six glob probes self-disable on the machine that most needed them: applying
|
||||
# the one-character typo `[**/SKILL.md]` -> `[**/SKILLS.md]` and running with
|
||||
# vale off PATH exited 0, its sole output a stderr line pre-commit swallows,
|
||||
# so the pre-push hook reported `Passed`. That is the exact defect the
|
||||
# glob-coverage section exists to catch, disabled by the absence of the tool
|
||||
# that catches it. Assert the MESSAGE: exit 1 has a dozen causes here and the
|
||||
# fixture is in sync, so the code alone would not distinguish this from any
|
||||
# other finding.
|
||||
NOVALE_OUT=""
|
||||
NOVALE_RC=0
|
||||
NOVALE_OUT="$(PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE17" 2>&1)" || NOVALE_RC=$?
|
||||
if [[ $NOVALE_RC -eq 0 ]]; then
|
||||
fail "exited 0 on in-sync copies with vale unavailable — a run that could not verify glob coverage must not report success"
|
||||
elif ! printf '%s\n' "$NOVALE_OUT" | grep -q "vale is not installed, so none of the .vale.ini glob-coverage probes ran"; then
|
||||
fail "failed without vale for the wrong reason — the missing-binary guard did not fire: $(printf '%s' "$NOVALE_OUT" | tr '\n' ' ')"
|
||||
else
|
||||
fail "exited non-zero on in-sync copies with vale unavailable — the missing binary must warn, not fail"
|
||||
pass "hard-fails, saying so, when vale is unavailable and no opt-out is set"
|
||||
fi
|
||||
if PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE18" > /dev/null 2>&1; then
|
||||
|
||||
# 12b. The opt-out is the only way to get a clean exit without vale, and it has
|
||||
# to be set deliberately. Absence of the binary must never imply it.
|
||||
if PATH="$PATH_NO_VALE" CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 \
|
||||
bash "$SCRIPT" "$FIXTURE17" > /dev/null 2>&1; then
|
||||
pass "exits 0 on in-sync copies with vale unavailable and the explicit opt-out set"
|
||||
else
|
||||
fail "exited non-zero on in-sync copies with vale unavailable and CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 — the opt-out does not work"
|
||||
fi
|
||||
|
||||
# 12c/12d. The text assertions still gate under the opt-out. This is what the
|
||||
# opt-out has to preserve: masking vale makes the assertion under test the only
|
||||
# thing that can produce the verdict (with vale present, a dropped StylesPath
|
||||
# also breaks the probe, so these cases would still exit 1 with the assertion
|
||||
# itself deleted).
|
||||
if PATH="$PATH_NO_VALE" CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 \
|
||||
bash "$SCRIPT" "$FIXTURE18" > /dev/null 2>&1; then
|
||||
fail "exited 0 on a dropped StylesPath with vale unavailable — expected exit 1"
|
||||
else
|
||||
pass "exits non-zero on a dropped StylesPath with vale unavailable"
|
||||
fi
|
||||
if PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1; then
|
||||
if PATH="$PATH_NO_VALE" CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 \
|
||||
bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1; then
|
||||
fail "exited 0 on a BasedOnStyles that dropped Kyberforge with vale unavailable — expected exit 1"
|
||||
else
|
||||
pass "exits non-zero on a BasedOnStyles that dropped Kyberforge with vale unavailable"
|
||||
fi
|
||||
# A clean run without vale must say so — silence would read as verified.
|
||||
if PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE17" 2>&1 | grep -q "vale is not installed"; then
|
||||
pass "warns that glob coverage was not verified when vale is unavailable"
|
||||
|
||||
# 12e. An opted-out clean run must still say it verified nothing — otherwise
|
||||
# the opt-out just reintroduces the silent vacuous pass under a new name.
|
||||
OPTOUT_OUT="$(PATH="$PATH_NO_VALE" CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1 \
|
||||
bash "$SCRIPT" "$FIXTURE17" 2>&1)"
|
||||
if printf '%s\n' "$OPTOUT_OUT" | grep -q "glob coverage was NOT verified" \
|
||||
&& printf '%s\n' "$OPTOUT_OUT" | grep -q "0 glob probe(s) verified"; then
|
||||
pass "an opted-out clean run reports that glob coverage was not verified"
|
||||
else
|
||||
fail "exited clean without vale and said nothing — an unverified run looks identical to a verified one"
|
||||
fail "an opted-out clean run did not say it verified no glob coverage — it looks identical to a verified one: $(printf '%s' "$OPTOUT_OUT" | tr '\n' ' ')"
|
||||
fi
|
||||
|
||||
# 12f. The typo the whole section exists to catch must fail with vale absent
|
||||
# and the opt-out set, or not at all — never pass. It cannot be caught without
|
||||
# vale, so the opt-out must not turn it into a green run by accident: with the
|
||||
# opt-out this fixture legitimately passes, which is precisely why the opt-out
|
||||
# is gated on an env var and 12a is the default.
|
||||
FIXTURE19B="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE19B")
|
||||
break_glob "$FIXTURE19B/plugins/kyberforge/.apm/skills/skill-audit/assets/vale/.vale.ini" \
|
||||
'[**/SKILL.md]' '[**/SKILLS.md]'
|
||||
if PATH="$PATH_NO_VALE" bash "$SCRIPT" "$FIXTURE19B" > /dev/null 2>&1; then
|
||||
fail "the one-character glob typo exited 0 with vale off PATH — the probe self-disabled on the exact defect it exists to catch"
|
||||
else
|
||||
pass "the one-character glob typo does not exit 0 with vale off PATH"
|
||||
fi
|
||||
fi
|
||||
|
||||
|
||||
Reference in New Issue
Block a user