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
This commit is contained in:
@@ -9,15 +9,42 @@ FAIL=0
|
||||
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
||||
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
||||
|
||||
# Minimal fixture: a bare directory with no .git of its own. Since
|
||||
# Fixture: a temp directory that is its own git worktree. Since
|
||||
# scripts/sync-marketplace-mirror.sh resolves REPO_ROOT via
|
||||
# `git rev-parse --show-toplevel 2>/dev/null || pwd`, and mktemp -d creates
|
||||
# directories outside any git worktree, cd'ing into the fixture before
|
||||
# invoking the script makes REPO_ROOT resolve to the fixture itself -- so
|
||||
# every test runs against an isolated .claude-plugin/ + .github/plugin/ pair
|
||||
# instead of this repo's real marketplace.json files.
|
||||
# `git rev-parse --show-toplevel 2>/dev/null || pwd`, isolation has to come
|
||||
# from that call answering "the fixture" -- so the fixture owns a real .git.
|
||||
#
|
||||
# Relying instead on `git rev-parse` FAILING inside a bare `mktemp -d` (its
|
||||
# previous form) is not isolation: two ordinary conditions make it succeed and
|
||||
# resolve to the LIVE repo, at which point every test writes to this repo's own
|
||||
# tracked .github/plugin/marketplace.json. Both are reproduced and fixed here:
|
||||
# - TMPDIR pointing inside a git worktree, which puts the fixture in one.
|
||||
# - An inherited GIT_DIR/GIT_WORK_TREE, which re-targets `git -C` and
|
||||
# `git rev-parse` regardless of cwd. tests/run-tests.sh is itself a pre-push
|
||||
# hook, and git hooks export exactly those variables -- the same leak
|
||||
# tests/test-git-hooks-install.sh:6-10 already defends against.
|
||||
# run_script() strips GIT_DIR/GIT_WORK_TREE for the second; `git init` here
|
||||
# covers the first and makes the resolution positive rather than accidental.
|
||||
# run-tests.sh:52-53 asserts no test writes back into the live repo tree, and
|
||||
# that claim now carries the concurrent runner.
|
||||
make_fixture() {
|
||||
mktemp -d
|
||||
local dir
|
||||
dir="$(mktemp -d)"
|
||||
# Physical path: `git rev-parse --show-toplevel` reports the resolved path,
|
||||
# and on macOS `mktemp -d` hands back one under the /tmp -> /private/tmp
|
||||
# symlink. Without -P the script's REPO_ROOT and the assertions' $FIXTURE
|
||||
# would name the same directory differently and every diff would compare
|
||||
# against a path the script never wrote.
|
||||
dir="$(cd "$dir" && pwd -P)"
|
||||
# Checked, not best-effort: this init IS the isolation invariant. `echo "$dir"`
|
||||
# is the last statement, so a silently-failed init would return a plain temp
|
||||
# dir, `git rev-parse --show-toplevel` would walk up to whatever repo encloses
|
||||
# it, and the suite would go back to writing into the live tree.
|
||||
if ! env -u GIT_DIR -u GIT_WORK_TREE git -C "$dir" init -q >/dev/null 2>&1; then
|
||||
echo "make_fixture: 'git init' failed in $dir — every test would then resolve REPO_ROOT to an enclosing repo and write outside the fixture" >&2
|
||||
exit 1
|
||||
fi
|
||||
echo "$dir"
|
||||
}
|
||||
|
||||
SRC_REL=".claude-plugin/marketplace.json"
|
||||
@@ -37,10 +64,14 @@ write_dst() {
|
||||
printf '%s' "$content" > "$dir/$DST_REL"
|
||||
}
|
||||
|
||||
# `env -u GIT_DIR -u GIT_WORK_TREE` mirrors tests/test-git-hooks-install.sh:10:
|
||||
# under a git hook (run-tests.sh runs as pre-push) those are exported, and the
|
||||
# script's `git rev-parse --show-toplevel` would then answer with the LIVE repo
|
||||
# no matter which directory it was invoked from.
|
||||
run_script() {
|
||||
local dir="$1"
|
||||
shift
|
||||
(cd "$dir" && bash "$SCRIPT" "$@")
|
||||
(cd "$dir" && env -u GIT_DIR -u GIT_WORK_TREE bash "$SCRIPT" "$@")
|
||||
}
|
||||
|
||||
CLEANUP_DIRS=()
|
||||
@@ -67,6 +98,31 @@ else
|
||||
fail "missing source should exit 0 in --check mode, not report drift"
|
||||
fi
|
||||
|
||||
# --- 2b. Source missing but a mirror still present: --check must FAIL ---
|
||||
# --check used to exit 0 on any missing source, so deleting
|
||||
# .claude-plugin/marketplace.json left a stale .github/plugin/marketplace.json
|
||||
# reported as "no drift" -- a mirror of a file that no longer exists. That is
|
||||
# the silent divergence this script's header says it prevents ("keeps that
|
||||
# legacy mirror byte-identical ... instead of letting it silently drift"), and
|
||||
# scripts/sync-plugin-content.sh --check --all already errors on the same
|
||||
# condition. Case 2 above still holds: neither file present stays a no-op.
|
||||
echo ""
|
||||
echo "--- missing source with a surviving mirror: --check reports drift ---"
|
||||
FIXTURE2B="$(make_fixture)"; track "$FIXTURE2B"
|
||||
write_dst "$FIXTURE2B" "$CONTENT_A"
|
||||
if run_script "$FIXTURE2B" --check > /dev/null 2>&1; then
|
||||
fail "exited 0 with a stale mirror and no source -- expected drift (exit 1)"
|
||||
else
|
||||
pass "a mirror with no source left to mirror is reported as drift"
|
||||
fi
|
||||
# Real-sync mode keeps its no-op: it has nothing to copy, and deleting a
|
||||
# tracked file is not this script's call to make.
|
||||
if run_script "$FIXTURE2B" > /dev/null 2>&1 && [[ -f "$FIXTURE2B/$DST_REL" ]]; then
|
||||
pass "real-sync mode still no-ops on a missing source, leaving the mirror alone"
|
||||
else
|
||||
fail "real-sync mode should no-op on a missing source, not fail or delete the mirror"
|
||||
fi
|
||||
|
||||
# --- 3. Source exists, mirror missing entirely: --check reports drift (exit 1) ---
|
||||
echo ""
|
||||
echo "--- --check reports drift when the mirror file does not exist yet ---"
|
||||
@@ -179,6 +235,26 @@ else
|
||||
fail "a second sync run introduced unexpected drift"
|
||||
fi
|
||||
|
||||
# --- 12. Fixture isolation survives an inherited GIT_DIR/GIT_WORK_TREE ---
|
||||
# The whole suite's isolation is REPO_ROOT resolving to the fixture. With
|
||||
# GIT_DIR/GIT_WORK_TREE exported -- which is every git-hook context, and
|
||||
# run-tests.sh runs as pre-push -- `git rev-parse --show-toplevel` answers with
|
||||
# THAT repo from any cwd, so the script wrote to the live tree and 5 of these
|
||||
# cases failed. Point both variables at a decoy repo (never the live one, so
|
||||
# this assertion cannot itself write where it must not) and assert the fixture
|
||||
# still wins: the mirror lands in the fixture and the decoy stays untouched.
|
||||
echo ""
|
||||
echo "--- fixture isolation holds with GIT_DIR/GIT_WORK_TREE inherited from elsewhere ---"
|
||||
FIXTURE12="$(make_fixture)"; track "$FIXTURE12"
|
||||
DECOY="$(make_fixture)"; track "$DECOY"
|
||||
write_src "$FIXTURE12" "$CONTENT_A"
|
||||
if (export GIT_DIR="$DECOY/.git" GIT_WORK_TREE="$DECOY"; run_script "$FIXTURE12" > /dev/null 2>&1) \
|
||||
&& [[ -f "$FIXTURE12/$DST_REL" ]] && [[ ! -e "$DECOY/$DST_REL" ]]; then
|
||||
pass "an inherited GIT_DIR/GIT_WORK_TREE does not redirect writes out of the fixture"
|
||||
else
|
||||
fail "an inherited GIT_DIR/GIT_WORK_TREE redirected the sync outside the fixture"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
echo "Results: $PASS passed, $FAIL failed"
|
||||
[[ $FAIL -eq 0 ]]
|
||||
|
||||
Reference in New Issue
Block a user