diff --git a/scripts/check-vale-style-sync.sh b/scripts/check-vale-style-sync.sh index b1f876b..c6d6a7b 100755 --- a/scripts/check-vale-style-sync.sh +++ b/scripts/check-vale-style-sync.sh @@ -67,15 +67,31 @@ AGENT_INI="$AGENT_AUDIT/assets/vale/.vale.ini" for ini in "$SKILL_INI" "$AGENT_INI"; do rel_ini="${ini#"$REPO_ROOT"/}" - if [[ ! -f "$ini" ]]; then + # `-e`, not `-f`: a path that exists but is not a readable regular file (a + # directory sitting where the file should be, say) is not "missing", and + # reporting it as missing sends you looking for a deleted file. It belongs to + # the unreadable case below, which is the one that describes what actually + # went wrong. + if [[ ! -e "$ini" ]]; then err "$rel_ini is missing — without it vale falls back to an upward config search and lints with whatever it finds" continue fi # Present but unreadable is its own case: every assertion below is a grep, and # grep exits 2 on a read error. The override capture swallows that into an - # empty result, which would read as "no findings" rather than "not checked". - if [[ ! -r "$ini" ]]; then - err "$rel_ini is not readable — none of its assertions could run, and an unreadable file cannot be distinguished from a clean one downstream" + # empty result, which would read as "no findings" rather than "not checked", + # and the two greps above it report "has no StylesPath"/"names no Kyberforge" + # for a file that may well have both — a misdiagnosis, not a missed one. + # + # Decided by ACTUALLY READING the file, not by `[[ -r ]]`. `-r` is access(2), + # which answers "would the permission bits allow it" — and for uid 0 that is + # yes even on a mode-000 file (verified). This hook runs at pre-push, and this + # repo's dev environment is root, so an `[[ ! -r ]]` guard could never fire in + # the one place it exists to fire: it was untestable because it was dead. A + # read attempt is also the stricter question, catching EISDIR and EIO, which + # access(2) reports on neither. `cat`, not a bare `< "$ini"` redirect: opening + # a directory for reading succeeds, only the read fails. + if ! cat "$ini" >/dev/null 2>&1; then + err "$rel_ini exists but could not be read — none of its assertions could run, and an unreadable file cannot be distinguished from a clean one downstream" continue fi # StylesPath is resolved relative to the .vale.ini, which is the only reason diff --git a/tests/test-check-vale-style-sync.sh b/tests/test-check-vale-style-sync.sh index 76b4585..e656846 100755 --- a/tests/test-check-vale-style-sync.sh +++ b/tests/test-check-vale-style-sync.sh @@ -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"