Files
holocron/tests/test-check-scope-walkup-sync.sh
Defame1297 413a750819 fix(scripts): close gates that passed while the thing they guard was disabled
Four repo gates reported success in states they exist to reject.

`check-vale-style-sync.sh` passed while a Kyberforge lint rule was silenced. The
check matched a blocklist of severity values, but Vale's semantic is an allowlist:
anything that is not exactly YES/error/warning/suggestion disables the rule. So
`= false`, `= 0`, `= garbage`, an empty value and — worst — a lowercase `= yes` all
killed enforcement while reading as "enabled" to a human. Inverted to an allowlist.
Two sibling holes: dropping `KyberforgeCopilot` from `BasedOnStyles` unloaded the
Copilot-only check silently, and narrowing a section glob to a location made Vale
lint zero files, which is the "0 files, hook Passed" failure the script's own
comment says it exists to catch.

`sync-marketplace-mirror.sh --check` failed open when its source was missing, while
its sibling correctly errored in the same state.

`check-scope-walkup-sync.sh` wrote to hardcoded `/tmp/fN.out` paths and read one
back, making it non-reentrant — a concurrent instance can flip a verdict, and this
branch made the test runner concurrent. Now per-run `mktemp -d`.

`check-manifests.sh` had no disk-to-marketplace pass, so a plugin directory absent
from `marketplace.json` passed every gate while the `validate-plugins` hook globbed
it. The "listed" match is restricted to remote-source entry names; matching any
entry name let a genuine orphan through on a name coincidence.

`run-bats.sh` reported an empty TAP stream as `0 tests, 0 failures`, exit 0 — a
total harness failure reading as a pass.

The test-side changes are the larger half, because the guards were the real problem.
`test-sync-marketplace-mirror.sh` could overwrite the live tracked mirror under an
inherited GIT_DIR, which is precisely the git-hook context it runs in. The bash-3.2
scan hand-maintained its file list, omitting the new shared runner, and had no rule
for `wait -n` or `nproc` — the two hazards the previous review round found live. It
now derives 43 files across three globs with per-glob floors. Several assertions
were decoration: the concurrency checks caught the reentrancy defect 0 times in 10,
the leak fix was green either way, and two manifest fixtures passed with the code
they claimed to cover deleted. Every assertion now has a revert it provably fails
against.

Refs: #90

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
2026-08-14 01:52:56 +00:00

234 lines
11 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")
# --- 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
# --- 2. Exits 0 as a no-op when the kyberforge skills aren't present ---
echo ""
echo "--- exits 0 (no-op) when the target scripts don't exist ---"
FIXTURE_EMPTY="$(mktemp -d)"
FIXTURES+=("$FIXTURE_EMPTY")
if bash "$SCRIPT" "$FIXTURE_EMPTY" > /dev/null 2>&1; then
pass "exits 0 as a no-op when agent-author/agent-audit/skill-author aren't present"
else
fail "exited non-zero when the kyberforge skills are simply absent"
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. Builds a minimal
# REPO_ROOT (just the four scripts, at their real relative paths) so this
# doesn't depend on — or risk mutating — the real repo tree.
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"
}
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 ]]