From c442f7eb8507a6ca5e79b83705a888bad4a444a0 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Fri, 14 Aug 2026 08:03:58 +0000 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT --- scripts/check-vale-style-sync.sh | 24 ++++++-- tests/test-check-vale-style-sync.sh | 90 +++++++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 10 deletions(-) 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"