Defame1297
af085ed057
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
283 lines
14 KiB
Bash
Executable File
283 lines
14 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
|
|
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
|
SCRIPT="$REPO_ROOT/scripts/check-scope-walkup-sync.sh"
|
|
PASS=0
|
|
FAIL=0
|
|
|
|
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
|
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
|
|
|
FIXTURES=()
|
|
cleanup() { [[ ${#FIXTURES[@]} -eq 0 ]] || rm -rf "${FIXTURES[@]}"; }
|
|
trap cleanup EXIT
|
|
|
|
# Per-run scratch dir for the captured-output files below, for the same reason
|
|
# scripts/check-scope-walkup-sync.sh has one: tests/run-tests.sh fans its test
|
|
# scripts out concurrently, so a fixed path in the shared system temp directory
|
|
# is mutable state shared between two simultaneous runs. In FIXTURES above.
|
|
RUN_TMP="$(mktemp -d)"
|
|
FIXTURES+=("$RUN_TMP")
|
|
|
|
# Builds a minimal REPO_ROOT (just the four scripts, at their real relative
|
|
# paths) so the mutation cases below don't depend on — or risk mutating — the
|
|
# real repo tree. Defined up here rather than beside its first mutation case
|
|
# because case 2b's stale-.apm/ fixture is built from it too.
|
|
make_minimal_repo_root() {
|
|
local dir
|
|
dir="$(mktemp -d)"
|
|
local na="$dir/plugins/kyberforge/.apm/skills/agent-author/scripts"
|
|
local ns="$dir/plugins/kyberforge/.apm/skills/skill-author/scripts"
|
|
local aa="$dir/plugins/kyberforge/.apm/skills/agent-audit/scripts"
|
|
mkdir -p "$na" "$ns" "$aa"
|
|
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/agent-author/scripts/new-agent.sh" "$na/"
|
|
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/skill-author/scripts/new-skill.sh" "$ns/"
|
|
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh" "$aa/"
|
|
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate-provenance.sh" "$aa/"
|
|
# agent-author's templates are needed by new-agent.sh at runtime.
|
|
cp -R "$REPO_ROOT/plugins/kyberforge/.apm/skills/agent-author/assets" "$dir/plugins/kyberforge/.apm/skills/agent-author/"
|
|
cp -R "$REPO_ROOT/plugins/kyberforge/.apm/skills/skill-author/assets" "$dir/plugins/kyberforge/.apm/skills/skill-author/"
|
|
# validate.sh needs field-inventory.md
|
|
mkdir -p "$dir/plugins/kyberforge/.apm/skills/agent-audit/references"
|
|
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/agent-audit/references/field-inventory.md" \
|
|
"$dir/plugins/kyberforge/.apm/skills/agent-audit/references/"
|
|
echo "$dir"
|
|
}
|
|
|
|
# --- 1. Exits 0 against this repo's own (fixed) scripts ---
|
|
echo ""
|
|
echo "--- exits 0 against this repo's real scripts ---"
|
|
if bash "$SCRIPT" "$REPO_ROOT" > "$RUN_TMP/clean.out" 2>&1; then
|
|
pass "exits 0 against this repo's real scope walk-up scripts"
|
|
else
|
|
fail "exited non-zero against this repo's real (already-fixed) scripts"
|
|
sed 's/^/ /' "$RUN_TMP/clean.out"
|
|
fi
|
|
|
|
# --- 1b. Positive: that exit 0 was earned, not vacuous ---
|
|
# Every other case here runs against a synthetic fixture, and exit 0 is also
|
|
# what the script produces when it finds nothing to check at all. So the
|
|
# assertion above passes just as happily on a run that executed zero fixtures.
|
|
# The `ok:` lines are the record of work actually done; each of the seven
|
|
# fixtures emits at least one, so a floor of 7 catches a whole fixture going
|
|
# dark as well as the all-or-nothing case (13 at the time of writing — the floor
|
|
# is deliberately below that so adding assertions to a fixture doesn't churn it).
|
|
CLEAN_OKS="$(grep -c '^ ok:' "$RUN_TMP/clean.out" || true)"
|
|
if [[ "$CLEAN_OKS" -ge 7 ]]; then
|
|
pass "the clean run against this repo actually exercised its fixtures ($CLEAN_OKS ok assertions)"
|
|
else
|
|
fail "the clean run against this repo reported only $CLEAN_OKS ok assertions (expected at least one per fixture) — exit 0 without the fixtures having run means nothing was checked"
|
|
sed 's/^/ /' "$RUN_TMP/clean.out"
|
|
fi
|
|
|
|
# --- 2. Exits 0 as a no-op ONLY when there is no kyberforge plugin at all ---
|
|
# The no-op is scoped to a repo that never installed kyberforge. Case 2b below is
|
|
# its counterpart and the one that matters.
|
|
echo ""
|
|
echo "--- exits 0 (no-op) when there is no plugins/kyberforge at all ---"
|
|
FIXTURE_EMPTY="$(mktemp -d)"
|
|
FIXTURES+=("$FIXTURE_EMPTY")
|
|
if [[ -e "$FIXTURE_EMPTY/plugins/kyberforge" ]]; then
|
|
fail "the empty fixture unexpectedly has a plugins/kyberforge, so it does not exercise the no-kyberforge no-op"
|
|
elif bash "$SCRIPT" "$FIXTURE_EMPTY" > /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 the repo simply has no kyberforge plugin"
|
|
fi
|
|
|
|
# --- 2b. Exits 1, saying so, when plugins/kyberforge exists but the .apm/
|
|
# scripts under it do not ---
|
|
# The four target paths are hardcoded as plugins/kyberforge/.apm/skills/... with
|
|
# no floor under them: `mv plugins/kyberforge/.apm plugins/kyberforge/.apm2` hit
|
|
# the "not present, nothing to check" branch and exited 0, indistinguishable
|
|
# from "all four implementations agree" 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.
|
|
#
|
|
# Asserted on the MESSAGE, not just the code: this script exits 1 for any fixture
|
|
# disagreement too, so the code alone would not tell a stale path from a genuine
|
|
# walk-up regression — and those call for opposite fixes.
|
|
echo ""
|
|
echo "--- exits 1 and says so when plugins/kyberforge exists but .apm/ does not ---"
|
|
FIXTURE_STALE="$(make_minimal_repo_root)"
|
|
FIXTURES+=("$FIXTURE_STALE")
|
|
mv "$FIXTURE_STALE/plugins/kyberforge/.apm" "$FIXTURE_STALE/plugins/kyberforge/.apm2"
|
|
STALE_RC=0
|
|
bash "$SCRIPT" "$FIXTURE_STALE" > "$RUN_TMP/stale.out" 2>&1 || STALE_RC=$?
|
|
if [[ $STALE_RC -eq 0 ]]; then
|
|
fail "exited 0 when plugins/kyberforge exists but its .apm/ scripts are gone — expected exit 1"
|
|
elif ! grep -q "\.apm/ paths have gone stale" "$RUN_TMP/stale.out"; then
|
|
fail "failed for the wrong reason on a stale .apm/ path: $(tr '\n' ' ' < "$RUN_TMP/stale.out")"
|
|
else
|
|
pass "exits non-zero and reports a stale .apm/ path when plugins/kyberforge exists without it"
|
|
fi
|
|
|
|
# --- 3. Exits 1 against a REPO_ROOT that doesn't exist ---
|
|
echo ""
|
|
echo "--- exits 1 when REPO_ROOT does not exist ---"
|
|
if bash "$SCRIPT" "/nonexistent/path/$(date +%s)-$$" > /dev/null 2>&1; then
|
|
fail "exited 0 for a nonexistent REPO_ROOT — expected exit 1"
|
|
else
|
|
pass "exits non-zero for a nonexistent REPO_ROOT"
|
|
fi
|
|
|
|
# --- 4. Regression guard: reintroducing the $HOME-collapse bug into
|
|
# validate.sh's detect_scope must make the check fail.
|
|
echo ""
|
|
echo "--- exits 1 when validate.sh's detect_scope collapses back to the \$HOME-walk-up bug ---"
|
|
FIXTURE_BUG="$(make_minimal_repo_root)"
|
|
FIXTURES+=("$FIXTURE_BUG")
|
|
python3 - "$FIXTURE_BUG/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh" <<'PYTHON'
|
|
import re, sys
|
|
path = sys.argv[1]
|
|
with open(path) as f:
|
|
content = f.read()
|
|
# Revert to the pre-fix collapsed logic: both the $HOME-boundary case and the
|
|
# filesystem-root fallback return 'user', home unconditionally.
|
|
old = """def detect_scope(start_dir):
|
|
home = os.path.expanduser('~')
|
|
original_start = os.path.abspath(start_dir)"""
|
|
assert old in content, "detect_scope signature not found — validate.sh has changed shape"
|
|
buggy = '''def detect_scope(start_dir):
|
|
home = os.path.expanduser('~')
|
|
current = os.path.abspath(start_dir)
|
|
while True:
|
|
apm_yml = os.path.join(current, 'apm.yml')
|
|
if os.path.isfile(apm_yml) and find_apm_package_root(apm_yml):
|
|
return 'plugin', current
|
|
if current == home:
|
|
return 'user', home
|
|
if os.path.exists(os.path.join(current, '.git')):
|
|
return 'project', current
|
|
parent = os.path.dirname(current)
|
|
if parent == current:
|
|
return 'user', home
|
|
current = parent
|
|
'''
|
|
# Replace the whole function body up to (but not including) the next
|
|
# top-level `agent_dir = ` assignment that calls it.
|
|
pattern = re.compile(r"def detect_scope\(start_dir\):\n.*?\n(?=agent_dir = )", re.DOTALL)
|
|
assert pattern.search(content), "could not isolate detect_scope's full body"
|
|
content = pattern.sub(buggy + "\n", content)
|
|
with open(path, 'w') as f:
|
|
f.write(content)
|
|
PYTHON
|
|
if bash "$SCRIPT" "$FIXTURE_BUG" > "$RUN_TMP/buggy.out" 2>&1; then
|
|
fail "exited 0 against a validate.sh reverted to the \$HOME-collapse bug — expected exit 1"
|
|
else
|
|
pass "exits non-zero when validate.sh's detect_scope regresses to the \$HOME-collapse bug"
|
|
fi
|
|
|
|
echo ""
|
|
echo "--- exits 1 when validate-provenance.sh's find_plugin_root loses its \$HOME boundary check ---"
|
|
FIXTURE_BUG2="$(make_minimal_repo_root)"
|
|
FIXTURES+=("$FIXTURE_BUG2")
|
|
python3 - "$FIXTURE_BUG2/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate-provenance.sh" <<'PYTHON'
|
|
import re, sys
|
|
path = sys.argv[1]
|
|
with open(path) as f:
|
|
content = f.read()
|
|
# Drop the `if current == home: return None` line — reverts to the pre-fix
|
|
# behavior of never checking a $HOME boundary at all.
|
|
pattern = re.compile(r"\n *# \$HOME is a non-plugin-scope boundary.*?\n *if current == home:\n *return None\n", re.DOTALL)
|
|
assert pattern.search(content), "could not find the \\$HOME boundary check to remove"
|
|
content = pattern.sub("\n", content)
|
|
with open(path, 'w') as f:
|
|
f.write(content)
|
|
PYTHON
|
|
if bash "$SCRIPT" "$FIXTURE_BUG2" > "$RUN_TMP/buggy2.out" 2>&1; then
|
|
fail "exited 0 against a validate-provenance.sh with no \$HOME boundary check — expected exit 1"
|
|
else
|
|
pass "exits non-zero when validate-provenance.sh's find_plugin_root loses its \$HOME boundary check"
|
|
fi
|
|
|
|
# --- 6. Reentrancy ---
|
|
# The script and this test both used to capture output to fixed paths in the shared
|
|
# system temp directory. tests/run-tests.sh runs its scripts concurrently, so two
|
|
# instances shared those paths: the script's fixture 6 reads its capture back to
|
|
# assert it is empty, so a write from the other instance turned a passing fixture
|
|
# into a spurious FAIL, and a stale directory sitting at one of the paths broke the
|
|
# run outright ("Is a directory").
|
|
#
|
|
# THE SOURCE ASSERTION BELOW IS THE REGRESSION GUARD. The race itself is not usefully
|
|
# testable: 8 simultaneous instances of the broken script were measured exiting 0 with
|
|
# no FAIL lines, so a concurrent pair reproduces the defect approximately never. Only
|
|
# the "does either file name a shared temp path" invariant is deterministic, so that is
|
|
# what actually holds the fix in place -- for BOTH files, since this one had the same
|
|
# defect at 4 sites and was previously unguarded.
|
|
#
|
|
# Matching strategy: look for the shared temp directory anywhere on a line, then drop
|
|
# whole-line comments. The obvious alternative -- strip comments with `sed 's/#.*//'`
|
|
# and then match -- is wrong in this repo, because it truncates any line containing a
|
|
# ${var#prefix} expansion and would silently stop seeing a redirect that follows one.
|
|
# Comments that mention the shared temp path by name will trip this and have to be
|
|
# reworded; that is the safe direction to fail in.
|
|
SHARED_TMP_PATTERN='/'"tmp" # spelled by concatenation so this line cannot self-match
|
|
|
|
assert_no_shared_tmp() {
|
|
local label="$1" file="$2" out="$3"
|
|
# Without this, a missing file disarms the guard silently rather than failing:
|
|
# grep exits 2, the comment filter sees empty input and exits 1, and pipefail
|
|
# reports 2 -- a non-zero status, which is the "clean" branch below. A rename
|
|
# would then quietly retire the assertion instead of breaking the build.
|
|
if [[ ! -f "$file" ]]; then
|
|
fail "$label: cannot check for shared system-temp paths — '$file' does not exist"
|
|
return
|
|
fi
|
|
if grep -n "$SHARED_TMP_PATTERN" "$file" | grep -vE '^[0-9]+:[[:space:]]*#' > "$out"; then
|
|
fail "$label names a shared system-temp path — scratch files must live under a per-run mktemp -d"
|
|
sed 's/^/ /' "$out"
|
|
else
|
|
pass "$label names no shared system-temp paths"
|
|
fi
|
|
}
|
|
|
|
echo ""
|
|
echo "--- neither the script nor this test hardcodes a shared system-temp path ---"
|
|
assert_no_shared_tmp "check-scope-walkup-sync.sh" "$SCRIPT" "$RUN_TMP/hardcoded-script.out"
|
|
assert_no_shared_tmp "test-check-scope-walkup-sync.sh" "${BASH_SOURCE[0]}" "$RUN_TMP/hardcoded-test.out"
|
|
|
|
# The per-run scratch dir must be registered with the cleanup trap. This is a real,
|
|
# deterministic property (it fails if RUN_TMP is created but never added to FIXTURES)
|
|
# but note what it is NOT: it cannot detect the original defect, because a script
|
|
# writing to the shared temp directory directly never touches TMPDIR, leaving the
|
|
# probe dir empty by construction. It guards the cleanup wiring, not reentrancy.
|
|
echo ""
|
|
echo "--- the script's per-run scratch dir is cleaned up on exit ---"
|
|
SCRATCH_PROBE="$(mktemp -d)"
|
|
FIXTURES+=("$SCRATCH_PROBE")
|
|
TMPDIR="$SCRATCH_PROBE" bash "$SCRIPT" "$REPO_ROOT" > "$RUN_TMP/scratch-probe.out" 2>&1
|
|
LEFTOVER="$(find "$SCRATCH_PROBE" -mindepth 1 -maxdepth 1 | wc -l | tr -d ' ')"
|
|
if [[ "$LEFTOVER" == "0" ]]; then
|
|
pass "the run left no scratch directory behind (RUN_TMP is registered in FIXTURES)"
|
|
else
|
|
fail "$LEFTOVER scratch entries survived the run — the per-run scratch dir is not registered with the cleanup trap"
|
|
fi
|
|
|
|
# Smoke test only, deliberately kept despite not guarding the defect above: it is the
|
|
# one assertion that exercises two instances actually running at the same time, so it
|
|
# would still catch a coarse regression (e.g. a lockfile or a fixed fixture path that
|
|
# makes concurrent runs fail outright). It is NOT evidence the race is fixed.
|
|
echo ""
|
|
echo "--- smoke: two simultaneous runs both still exit 0 ---"
|
|
CONCURRENT_TMP="$(mktemp -d)"
|
|
FIXTURES+=("$CONCURRENT_TMP")
|
|
( TMPDIR="$CONCURRENT_TMP" bash "$SCRIPT" "$REPO_ROOT" > "$RUN_TMP/conc-a.out" 2>&1 ) &
|
|
PID_A=$!
|
|
( TMPDIR="$CONCURRENT_TMP" bash "$SCRIPT" "$REPO_ROOT" > "$RUN_TMP/conc-b.out" 2>&1 ) &
|
|
PID_B=$!
|
|
RC_A=0; wait "$PID_A" || RC_A=$?
|
|
RC_B=0; wait "$PID_B" || RC_B=$?
|
|
if [[ $RC_A -eq 0 && $RC_B -eq 0 ]]; then
|
|
pass "two simultaneous runs both exit 0"
|
|
else
|
|
fail "a simultaneous pair of runs did not both exit 0 (a=$RC_A b=$RC_B)"
|
|
sed 's/^/ A: /' "$RUN_TMP/conc-a.out"
|
|
sed 's/^/ B: /' "$RUN_TMP/conc-b.out"
|
|
fi
|
|
|
|
echo ""
|
|
echo "Results: $PASS passed, $FAIL failed"
|
|
[[ $FAIL -eq 0 ]]
|