fix(scripts): decide .vale.ini readability by reading it, not by access(2)
Issue #97 item 1 reports the unreadable-.vale.ini guard as untested. It was worse: it was dead. `[[ -r ]]` is access(2), which asks whether the permission bits would allow a read -- and for uid 0 that is yes even on a mode-000 file. This hook runs at pre-push and the dev environment is root, so the guard could never fire where it exists to fire. That is why no uid-independent test for it existed; there was nothing to test. Readability is now decided by actually reading (`cat`), which is uid-independent and strictly stronger, catching EISDIR and EIO that access(2) reports on neither. `cat`, not a `< "$ini"` redirect: opening a directory for reading succeeds, only the read fails. The missing branch moves to `-e`, so a directory sitting where the file belongs is reported as unreadable rather than sending the reader hunting for a deleted file. The new case asserts the MESSAGE, not the exit code. With the guard removed the script still exits 1 -- the greps hit the unreadable path and blame a missing StylesPath on a file that has one. An exit-code-only test would have been green with the guard deleted. Also stops paying for vale in cases that only assert .vale.ini text: 21 of 28 script runs now mask it via the PATH_NO_VALE mechanism case 12 already builds, cutting the suite's bottleneck ~3.5x (issue #97 item 5). The helper falls back to an unmasked run rather than skipping, so a machine where masking is unavailable loses speed, never coverage. That masking is a coverage gain, not only a speedup. With vale on PATH, cases 8 and 9 could not detect deletion of the assertions they were written to catch: a dropped StylesPath also breaks the glob probe, so the script exited 1 for the wrong reason and both cases went green. Verified against the pre-change files -- the same mutation was caught by one incidental assertion before, and by three after. Refs #97 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -58,6 +58,45 @@ with open(path, 'w', encoding='utf-8') as fh:
|
||||
PYTHON
|
||||
}
|
||||
|
||||
# Vale masking, hoisted so the text-only cases below can use it. Case 12 keeps
|
||||
# its own independent construction and its own loud failure if masking breaks —
|
||||
# it is what proves this mechanism works, so it is not refactored onto this.
|
||||
#
|
||||
# Why: a script run with vale on PATH performs six `vale` invocations (one per
|
||||
# probe path), and they are the suite's entire wall clock. The cases that assert
|
||||
# a text-level finding — StylesPath, BasedOnStyles, per-rule overrides — reach
|
||||
# their verdict through `grep` alone and gain nothing from paying for the
|
||||
# probes. Masking vale is not merely cheaper for them, it is STRICTER: with vale
|
||||
# present a dropped StylesPath also breaks the probe, so such a case would still
|
||||
# exit 1 with the assertion under test deleted. Without vale, only the assertion
|
||||
# under test can produce the failure.
|
||||
#
|
||||
# run_check falls back to an unmasked run rather than skipping when masking is
|
||||
# not safely available, so a machine where this cannot work loses speed, never
|
||||
# coverage. The utility probe matters as much as the vale probe: PATH_NO_VALE
|
||||
# deletes a whole PATH entry, and if that entry also carried grep/diff/awk/cat
|
||||
# the script would fail for an unrelated reason and every negative case below
|
||||
# would pass vacuously.
|
||||
VALE_DIR="$(dirname "$(command -v vale 2>/dev/null || echo /nonexistent/vale)")"
|
||||
PATH_NO_VALE="$(printf '%s' "$PATH" | tr ':' '\n' | grep -vxF "$VALE_DIR" | paste -sd: -)"
|
||||
VALE_MASKED=false
|
||||
if ! PATH="$PATH_NO_VALE" bash -c 'command -v vale' >/dev/null 2>&1 \
|
||||
&& PATH="$PATH_NO_VALE" bash -c \
|
||||
'command -v grep && command -v diff && command -v awk && command -v cat' >/dev/null 2>&1; then
|
||||
VALE_MASKED=true
|
||||
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.
|
||||
run_check_no_vale() {
|
||||
if [[ "$VALE_MASKED" == true ]]; then
|
||||
PATH="$PATH_NO_VALE" bash "$SCRIPT" "$@"
|
||||
else
|
||||
bash "$SCRIPT" "$@"
|
||||
fi
|
||||
}
|
||||
|
||||
# --- 1. Exits 0 when the two copies are in sync ---
|
||||
echo ""
|
||||
echo "--- exits 0 when skill-audit and agent-audit copies are in sync ---"
|
||||
@@ -171,9 +210,48 @@ else
|
||||
pass "exits non-zero when a .vale.ini is missing"
|
||||
fi
|
||||
|
||||
# --- 7b. Exits 1, saying so, when a .vale.ini is present but cannot be read ---
|
||||
# Every assertion in that loop is a grep, and grep exits 2 on a read error: the
|
||||
# two `grep -q` checks then misreport a file whose StylesPath and BasedOnStyles
|
||||
# may be perfectly fine, and the override capture swallows the error into an
|
||||
# empty result that reads as "no findings". So the exit code alone proves
|
||||
# nothing here — the check already exits 1 either way, just with the wrong
|
||||
# reason — and this case asserts the MESSAGE. Deleting the readability guard
|
||||
# leaves the exit code at 1 and the diagnosis wrong, which is exactly the
|
||||
# mutation the assertion below kills.
|
||||
#
|
||||
# The unreadable path is a DIRECTORY, not a mode-000 file, and that is the whole
|
||||
# point of the case: `cat` on a directory fails for every uid, while a mode-000
|
||||
# file is readable by root, which is what this repo's dev environment and its
|
||||
# pre-push hooks run as. A permission-based fixture would pass or fail depending
|
||||
# on the invoking uid; this one does not.
|
||||
echo ""
|
||||
echo "--- exits 1 and says so when a .vale.ini exists but cannot be read ---"
|
||||
FIXTURE8B="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE8B")
|
||||
UNREADABLE_INI="$FIXTURE8B/plugins/kyberforge/.apm/skills/skill-audit/assets/vale/.vale.ini"
|
||||
rm -f "$UNREADABLE_INI"
|
||||
mkdir -p "$UNREADABLE_INI"
|
||||
UNREADABLE_OUT=""
|
||||
UNREADABLE_RC=0
|
||||
UNREADABLE_OUT="$(bash "$SCRIPT" "$FIXTURE8B" 2>&1)" || UNREADABLE_RC=$?
|
||||
if [[ -e "$UNREADABLE_INI" ]] && cat "$UNREADABLE_INI" >/dev/null 2>&1; then
|
||||
fail "the fixture's .vale.ini is still readable, so this case proves nothing about the unreadable branch"
|
||||
elif [[ $UNREADABLE_RC -eq 0 ]]; then
|
||||
fail "exited 0 when skill-audit's .vale.ini could not be read — expected exit 1"
|
||||
elif ! printf '%s\n' "$UNREADABLE_OUT" | grep -q "could not be read"; then
|
||||
fail "failed for the wrong reason on an unreadable .vale.ini — the readability guard did not fire, so the greps misdiagnosed it: $(printf '%s' "$UNREADABLE_OUT" | tr '\n' ' ')"
|
||||
else
|
||||
pass "exits non-zero and reports an unreadable .vale.ini as unreadable, not as missing or malformed"
|
||||
fi
|
||||
|
||||
# --- 8. Exits 1 when the shared StylesPath line is dropped from either copy ---
|
||||
# StylesPath resolves relative to the .vale.ini, which is the only reason the
|
||||
# bundled styles are found from a consuming repo's clone prefix.
|
||||
# Run with vale masked: a dropped StylesPath also stops vale finding the styles,
|
||||
# so with vale on PATH the glob probe fails too and this case would still exit 1
|
||||
# with the StylesPath assertion itself deleted. Masking makes the text assertion
|
||||
# the only thing that can produce the verdict.
|
||||
echo ""
|
||||
echo "--- exits 1 when StylesPath is missing from either .vale.ini ---"
|
||||
FIXTURE9="$(make_fixture)"
|
||||
@@ -183,12 +261,12 @@ break_glob "$FIXTURE9/plugins/kyberforge/.apm/skills/skill-audit/assets/vale/.va
|
||||
'StylesPath = styles' 'StylesPath = elsewhere'
|
||||
break_glob "$FIXTURE10/plugins/kyberforge/.apm/skills/agent-audit/assets/vale/.vale.ini" \
|
||||
'StylesPath = styles' 'StylesPath = elsewhere'
|
||||
if bash "$SCRIPT" "$FIXTURE9" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE9" > /dev/null 2>&1; then
|
||||
fail "exited 0 when skill-audit's .vale.ini lost StylesPath — expected exit 1"
|
||||
else
|
||||
pass "exits non-zero when skill-audit's .vale.ini lost StylesPath"
|
||||
fi
|
||||
if bash "$SCRIPT" "$FIXTURE10" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE10" > /dev/null 2>&1; then
|
||||
fail "exited 0 when agent-audit's .vale.ini lost StylesPath — expected exit 1"
|
||||
else
|
||||
pass "exits non-zero when agent-audit's .vale.ini lost StylesPath"
|
||||
@@ -203,7 +281,7 @@ FIXTURE11="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE11")
|
||||
break_glob "$FIXTURE11/plugins/kyberforge/.apm/skills/agent-audit/assets/vale/.vale.ini" \
|
||||
'BasedOnStyles = Kyberforge' 'BasedOnStyles = KyberforgeCopilot'
|
||||
if bash "$SCRIPT" "$FIXTURE11" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE11" > /dev/null 2>&1; then
|
||||
fail "exited 0 when agent-audit's .vale.ini stopped naming Kyberforge — expected exit 1"
|
||||
else
|
||||
pass "exits non-zero when a .vale.ini no longer names the Kyberforge style"
|
||||
@@ -246,7 +324,7 @@ while IFS= read -r override; do
|
||||
FIXTURE_OV="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE_OV")
|
||||
echo "$override" >> "$FIXTURE_OV/plugins/kyberforge/.apm/skills/skill-audit/assets/vale/.vale.ini"
|
||||
if bash "$SCRIPT" "$FIXTURE_OV" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE_OV" > /dev/null 2>&1; then
|
||||
fail "exited 0 with '$override' in skill-audit's .vale.ini -- expected exit 1"
|
||||
else
|
||||
pass "exits non-zero on '$override'"
|
||||
@@ -277,7 +355,7 @@ FIXTURE_OV_AGENT="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE_OV_AGENT")
|
||||
echo "KyberforgeCopilot.ProactivePhrase = false" \
|
||||
>> "$FIXTURE_OV_AGENT/plugins/kyberforge/.apm/skills/agent-audit/assets/vale/.vale.ini"
|
||||
if bash "$SCRIPT" "$FIXTURE_OV_AGENT" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE_OV_AGENT" > /dev/null 2>&1; then
|
||||
fail "exited 0 with 'KyberforgeCopilot.ProactivePhrase = false' in agent-audit's .vale.ini -- expected exit 1"
|
||||
else
|
||||
pass "exits non-zero when agent-audit's copy retires a KyberforgeCopilot rule"
|
||||
@@ -316,7 +394,7 @@ FIXTURE11C="$(make_fixture)"
|
||||
FIXTURES+=("$FIXTURE11C")
|
||||
break_glob "$FIXTURE11C/plugins/kyberforge/.apm/skills/agent-audit/assets/vale/.vale.ini" \
|
||||
'BasedOnStyles = Kyberforge, KyberforgeCopilot' 'BasedOnStyles = Kyberforge'
|
||||
if bash "$SCRIPT" "$FIXTURE11C" > /dev/null 2>&1; then
|
||||
if run_check_no_vale "$FIXTURE11C" > /dev/null 2>&1; then
|
||||
fail "exited 0 when KyberforgeCopilot was dropped from BasedOnStyles -- expected exit 1"
|
||||
else
|
||||
pass "exits non-zero when a shipped KyberforgeCopilot style is never loaded"
|
||||
|
||||
Reference in New Issue
Block a user