Defame1297/holocron
0
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases 3 Wiki Activity
Files
c5d43b4d63878cb233bd1cad3b2982c02be3d512
holocron/tests/test-apm-current-hook.sh
Defame1297 ffcbed6c41 fix(tests): replace pipefail-racy echo | grep -q with here-strings 
Why

Two suites failed intermittently — tests/test-vale-wrap.sh case 21 and
tests/test-check-release-needed.sh cases 4 and 15 — on correct output, and never
when run alone. The cause is the `echo "$OUT" | grep -q P` idiom under
`set -o pipefail`: grep -q exits as soon as it has an answer, bash's echo can
hand a multi-line value to the pipe one line at a time, and a write after the
reader is gone kills echo with SIGPIPE. pipefail then reports the writer's
death, so output that DID match reads as "no match". Every observed failure had
lines after its match; case 15's match is on line 1 of 6, the widest window in
that file.

Forced with a pause before the writer's last line, the pipe form failed 50 of 50
runs; a here-string, a match on the last line, and the same pipe without
pipefail each passed 50 of 50. Unforced the rate is about 1 per 670 suite runs,
which is why it read as a flaky gate rather than a bug.

The failures at review time are consistent with this, but were not proven to be
it: the suite was running while agents edited live config files in place, and a
brief change to .vale.ini or .pre-commit-hooks.yaml would produce the same two
failures. The race is real and fixed either way.

Implementation Notes

`grep -q P <<< "$VAR"` has no separate writer process, so there is nothing to
race. It is not a retry or a sleep. 121 sites converted across 9 files, three of
them scripts rather than tests: new-agent.sh, new-skill.sh and
check-executables-allow-sync.sh. None ships via .pre-commit-hooks.yaml, so no
external consumer pins them, and all three are single-pipeline checks whose
verdict cannot change.

Left alone deliberately: 14 sites whose writer is a command, not a shell
builtin — they either absorb the writer's status with `|| true` or are python3
and awk, which write once at exit — and one file with no pipefail. `printf '%s'`
sites differ from a here-string only by a trailing newline, which no -q verdict
on a non-empty pattern depends on.

tests/test-no-pipefail-early-exit-grep.sh is a static guard against new
occurrences, discovered automatically by run-tests.sh. It only scans files that
set pipefail, joins continuation lines, skips comments, and flags only
echo/printf writers. Its first case proves the scanner can fail before its
second trusts a clean verdict on the tree.

A guard covers exactly the spellings its regex models, so the miss surface was
measured rather than assumed. Four were found and closed: pipefail declared as
`set -o errexit -o pipefail` (where the old pattern required pipefail to follow
the FIRST -o, and a file-level miss skips every site in that file); a writer
separated from grep by an intermediate stage; a pipeline wrapped on a trailing
`|` rather than a backslash; and readers spelled egrep, fgrep, /bin/grep,
`command grep` or with an env-var prefix. Segment characters exclude a bare `&`
so `echo ok && other | grep -q x`, whose writer is `other`, does not false-fire.
Widening surfaced 5 live sites invisible to the original scanner, all in
tests/test-apm-current-hook.sh, all `echo "$out" | json_field ... | grep -q`;
they are safe today only because json_field is python3, which reads to EOF and
writes once. Fixtures go 4 to 12 vulnerable spellings plus near-miss negatives.

Two `grep ... | head -1` sites (test-vale-wrap.sh) are the same race with a
different early-exiting reader, and are fixed by absorbing the writer. The
scanner deliberately does not model `head`, `sed -n 1p` or a bare `read`: most
legitimate uses in this tree are already absorbed with `|| true` and the scanner
cannot see absorption from pipeline text, so a high false-positive rate would be
how this guard gets weakened. Heredoc bodies are scanned as code; none in the
tree trips it today.

Impact

The bug predates the factory-audit merge: every converted site in
check-release-needed and case 21 dates to 4d018af and aa8cc22 (2026-08-09).

Test suites go 19 to 20. `run-tests.sh --strict` passes 20/20 with 0 skipped,
four consecutive runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD
2026-09-16 09:14:01 +00:00

369 lines
17 KiB
Bash
Executable File
Raw Blame History

#!/usr/bin/env bash
# Tests for plugins/kyberforge/.apm/hooks/check-apm-current.sh — the SessionStart
# hook that keeps an apm-consumed install level with its remote.
#
# `apm` is mocked throughout: the hook's contract is "read `apm outdated`, decide,
# emit SessionStart JSON", and that is testable without a network or a real
# install.
set -euo pipefail
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
HOOK="$REPO_ROOT/plugins/kyberforge/.apm/hooks/check-apm-current.sh"
HOOKS_JSON="$REPO_ROOT/plugins/kyberforge/.apm/hooks/hooks.json"
PASS=0
FAIL=0
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
command -v python3 > /dev/null 2>&1 || { echo "python3 required"; exit 77; }
FAKE_BIN="$(mktemp -d)"
WORK="$(mktemp -d)"
trap 'rm -rf "$FAKE_BIN" "$WORK"' EXIT
# Mock `apm`. $1 chooses what `apm outdated` reports; $2 the exit code of
# `apm update`. Sentinel files record whether update was actually invoked and
# which directory the calls ran in — the hook must run them against the same
# directory its lockfile guard checked, not the session's cwd.
make_apm() {
local outdated_line="$1" update_exit="$2"
cat > "$FAKE_BIN/apm" << EOF
#!/usr/bin/env bash
pwd > "$WORK/apm-cwd"
case "\$1" in
outdated) echo "$outdated_line"; exit 0 ;;
update) touch "$WORK/update-was-called"; exit $update_exit ;;
esac
exit 0
EOF
chmod +x "$FAKE_BIN/apm"
}
# CLAUDE_PROJECT_DIR is cleared rather than merely left alone: a session in this
# repo exports it, and an inherited value would point every case at the real
# repo root (which has a real apm.lock.yaml) instead of the fixture. The
# project-directory cases below set it deliberately.
run_hook() { (cd "$WORK" && env -u CLAUDE_PROJECT_DIR PATH="$FAKE_BIN:$PATH" bash "$HOOK" 2>/dev/null); }
# Same, with an explicit cwd and CLAUDE_PROJECT_DIR. $1 is the cwd; $2 the value
# for CLAUDE_PROJECT_DIR, or the literal `-` to leave it unset.
run_hook_in() {
local cwd="$1" project_dir="$2"
if [[ "$project_dir" == "-" ]]; then
(cd "$cwd" && env -u CLAUDE_PROJECT_DIR PATH="$FAKE_BIN:$PATH" bash "$HOOK" 2>/dev/null)
else
(cd "$cwd" && env CLAUDE_PROJECT_DIR="$project_dir" PATH="$FAKE_BIN:$PATH" bash "$HOOK" 2>/dev/null)
fi
}
json_field() { python3 -c 'import json,sys; print(json.load(sys.stdin)["hookSpecificOutput"][sys.argv[1]])' "$1"; }
# ---------------------------------------------------------------------------
echo "--- inert without an apm-consumed install ---"
# ---------------------------------------------------------------------------
make_apm "[!] 6 outdated dependencies found" 0
rm -f "$WORK/apm.lock.yaml" "$WORK/update-was-called"
out="$(run_hook)"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 with no apm.lock.yaml" || fail "should exit 0 with no apm.lock.yaml"
[[ -z "$out" ]] && pass "emits nothing with no apm.lock.yaml" || fail "should stay silent with no apm.lock.yaml"
[[ ! -f "$WORK/update-was-called" ]] && pass "does not run apm update with no apm.lock.yaml" \
|| fail "must not touch a project that does not use apm"
# A native (non-apm) install of this plugin hits exactly this path, so it is the
# guard that keeps the hook from acting on someone else's repo.
touch "$WORK/apm.lock.yaml"
# ---------------------------------------------------------------------------
echo ""
echo "--- inert when apm is absent ---"
# ---------------------------------------------------------------------------
rm -f "$WORK/update-was-called"
out="$( (cd "$WORK" && PATH="$(dirname "$(command -v bash)")" bash "$HOOK" 2>/dev/null) )"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 when apm is not on PATH" || fail "should exit 0 when apm is missing"
[[ -z "$out" ]] && pass "emits nothing when apm is not on PATH" || fail "should stay silent when apm is missing"
# ---------------------------------------------------------------------------
echo ""
echo "--- install already current ---"
# ---------------------------------------------------------------------------
make_apm "[*] All dependencies are up-to-date" 0
rm -f "$WORK/update-was-called"
out="$(run_hook)"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 when current" || fail "should exit 0 when current"
[[ -z "$out" ]] && pass "emits nothing when current" || fail "should stay silent when current"
[[ ! -f "$WORK/update-was-called" ]] && pass "does not run apm update when current" \
|| fail "must not update when nothing is stale"
# ---------------------------------------------------------------------------
echo ""
echo "--- stale, refresh succeeds ---"
# ---------------------------------------------------------------------------
make_apm "[!] 6 outdated dependencies found" 0
rm -f "$WORK/update-was-called"
out="$(run_hook)"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 when stale" || fail "should exit 0 when stale"
[[ -f "$WORK/update-was-called" ]] && pass "runs apm update when stale" || fail "should run apm update when stale"
if echo "$out" | python3 -m json.tool > /dev/null 2>&1; then
pass "emits valid JSON"
[[ "$(echo "$out" | json_field hookEventName)" == "SessionStart" ]] \
&& pass "declares hookEventName SessionStart" || fail "wrong hookEventName"
[[ "$(echo "$out" | json_field reloadSkills)" == "True" ]] \
&& pass "asks the host to reload skills after a successful refresh" || fail "reloadSkills should be true"
grep -q "6 package" <<< "$(json_field additionalContext <<< "$out")" \
&& pass "reports the stale package count" || fail "should report the count"
# The lockfile rewrite is the surprising part of auto-updating; the notice has
# to say so or a dirty worktree looks like something else went wrong.
grep -q "apm.lock.yaml" <<< "$(json_field additionalContext <<< "$out")" \
&& pass "warns that apm.lock.yaml was rewritten" || fail "should warn about the lockfile rewrite"
else
fail "emits valid JSON"
fi
# ---------------------------------------------------------------------------
echo ""
echo "--- stale, refresh fails ---"
# ---------------------------------------------------------------------------
make_apm "[!] 3 outdated dependencies found" 1
out="$(run_hook)"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 when the refresh fails" || fail "must never fail the session start"
if echo "$out" | python3 -m json.tool > /dev/null 2>&1; then
pass "emits valid JSON on failure"
[[ "$(echo "$out" | json_field reloadSkills)" == "False" ]] \
&& pass "does not ask for a skill reload when nothing was deployed" || fail "reloadSkills should be false"
grep -q "apm update" <<< "$(json_field additionalContext <<< "$out")" \
&& pass "tells the reader how to refresh by hand" || fail "should name the manual command"
else
fail "emits valid JSON on failure"
fi
# ---------------------------------------------------------------------------
echo ""
echo "--- unparseable count degrades instead of breaking the JSON ---"
# ---------------------------------------------------------------------------
make_apm "[!] lots of outdated dependencies found" 0
out="$(run_hook)"
echo "$out" | python3 -m json.tool > /dev/null 2>&1 \
&& pass "still emits valid JSON when the count cannot be parsed" || fail "JSON broke on an unparseable count"
# ---------------------------------------------------------------------------
echo ""
echo "--- anchors on the project root, not the session cwd ---"
# ---------------------------------------------------------------------------
# `[[ -f apm.lock.yaml ]]` resolves against the cwd, and a session started in a
# subdirectory of an apm-consuming repo therefore no-opped silently — and would
# have run `apm outdated`/`apm update` against that wrong directory had the
# guard passed. Claude Code exports CLAUDE_PROJECT_DIR for SessionStart hooks,
# so that is the anchor; the cwd is only the fallback.
ELSEWHERE="$WORK/elsewhere"
mkdir -p "$ELSEWHERE"
rm -f "$ELSEWHERE/apm.lock.yaml"
make_apm "[!] 6 outdated dependencies found" 0
rm -f "$WORK/update-was-called" "$WORK/apm-cwd"
out="$(run_hook_in "$ELSEWHERE" "$WORK")"
[[ -f "$WORK/update-was-called" ]] \
&& pass "finds the lockfile via CLAUDE_PROJECT_DIR when the cwd is elsewhere" \
|| fail "a session started in a subdirectory must still see the project's lockfile"
[[ "$(cat "$WORK/apm-cwd" 2>/dev/null)" == "$WORK" ]] \
&& pass "runs apm in the directory the guard checked, not the cwd" \
|| fail "apm ran in '$(cat "$WORK/apm-cwd" 2>/dev/null)' — must run in the resolved project directory"
grep -q "6 package" <<< "$(json_field additionalContext <<< "$out")" \
&& pass "reports the count found via CLAUDE_PROJECT_DIR" || fail "should report the count"
# The fallback is not cosmetic: a host that installed this plugin natively sets
# no CLAUDE_PROJECT_DIR, and the hook must stay inert-but-harmless there rather
# than erroring on an unset variable (the script runs under `set -u`).
rm -f "$WORK/update-was-called" "$WORK/apm-cwd"
out="$(run_hook_in "$WORK" "-")"
[[ -f "$WORK/update-was-called" ]] \
&& pass "falls back to the cwd when CLAUDE_PROJECT_DIR is unset" \
|| fail "must still work with no CLAUDE_PROJECT_DIR in the environment"
[[ "$(cat "$WORK/apm-cwd" 2>/dev/null)" == "$WORK" ]] \
&& pass "runs apm in the cwd under the fallback" \
|| fail "apm ran in '$(cat "$WORK/apm-cwd" 2>/dev/null)' — should be the cwd"
rm -f "$WORK/update-was-called" "$WORK/apm-cwd"
out="$(run_hook_in "$ELSEWHERE" "$ELSEWHERE")"; rc=$?
[[ $rc -eq 0 ]] && pass "exits 0 when neither the project dir nor the cwd has a lockfile" \
|| fail "should exit 0 when there is no lockfile anywhere"
[[ -z "$out" ]] && pass "stays silent when neither the project dir nor the cwd has a lockfile" \
|| fail "should stay silent when there is no lockfile anywhere"
[[ ! -f "$WORK/update-was-called" ]] \
&& pass "does not run apm update when there is no lockfile anywhere" \
|| fail "must not touch a project that does not use apm"
# ---------------------------------------------------------------------------
echo ""
echo "--- hooks.json wiring ---"
# ---------------------------------------------------------------------------
# apm resolves script paths relative to the package root, and `apm pack` keeps
# only *.json from .apm/hooks/ — so a ${CLAUDE_PLUGIN_ROOT}/hooks/... reference
# points at a directory the script never reaches. It must be .apm/-relative.
referenced="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d["hooks"]["SessionStart"][0]["hooks"][0]["command"])' "$HOOKS_JSON")"
[[ "$referenced" == '${CLAUDE_PLUGIN_ROOT}/.apm/hooks/check-apm-current.sh' ]] \
&& pass "hooks.json references the script at its .apm/ path" \
|| fail "hooks.json references '$referenced' — must be \${CLAUDE_PLUGIN_ROOT}/.apm/hooks/check-apm-current.sh"
[[ -x "$HOOK" ]] && pass "hook script is executable" || fail "hook script must be executable"
matcher="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d["hooks"]["SessionStart"][0]["matcher"])' "$HOOKS_JSON")"
[[ "$matcher" == "startup" ]] && pass "fires on startup only" \
|| fail "matcher is '$matcher' — resume/clear/compact would re-run this every compaction"
# The host's timeout must strictly exceed everything the script can spend, or
# the host SIGKILLs the hook mid-`apm update` and leaves a half-redeployed
# .claude/skills/ with no notice emitted — the silent failure this hook exists
# to prevent. Asserted as an invariant over both files rather than against a
# literal, so raising either internal `timeout` without raising the host budget
# fails here instead of reintroducing the gap quietly.
#
# Every `timeout N` in the script counts, comments included: a stray "timeout
# 300" in prose only makes this stricter, which is the safe direction.
script_budget=0
timeout_count=0
while read -r n; do
[[ -n "$n" ]] || continue
script_budget=$((script_budget + n))
timeout_count=$((timeout_count + 1))
done < <(grep -oE '\btimeout [0-9]+\b' "$HOOK" | grep -oE '[0-9]+')
hook_timeout="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d["hooks"]["SessionStart"][0]["hooks"][0]["timeout"])' "$HOOKS_JSON")"
if [[ $timeout_count -eq 0 ]]; then
fail "found no 'timeout N' in $HOOK — the budget assertion below would be vacuous"
else
pass "parsed $timeout_count internal timeout(s) totalling ${script_budget}s from the hook script"
[[ $hook_timeout -gt $script_budget ]] \
&& pass "hooks.json timeout (${hook_timeout}s) exceeds the script's own budget (${script_budget}s)" \
|| fail "hooks.json timeout is ${hook_timeout}s but the script can spend ${script_budget}s — the host would SIGKILL it mid-update"
fi
# ---------------------------------------------------------------------------
echo ""
echo "--- the greped phrase, against the real apm ---"
# ---------------------------------------------------------------------------
# Everything above mocks `apm`, so an apm release that reworded its summary line
# would revert this hook to its pre-fix behaviour with a fully green suite.
# `apm outdated` has no --json/machine-readable flag (verified against 0.28.0),
# so the phrase match cannot be replaced — it can only be pinned.
#
# The probe stages a genuinely outdated dependency with no network: a local git
# repo stands in for the upstream, reached through `url.<path>.insteadOf`
# rewrites of every URL spelling apm may build (it picks ssh or https depending
# on ambient auth config, so all three are mapped). apm appends `.git` to the
# repo URL, which is why the local repo is named `upstream.git` and the rewrite
# target omits the suffix. HOME is redirected so no user-level apm cache or
# credential state can influence the result.
#
# The genuine output is then replayed into the real hook through the mock, so
# what is asserted is the hook's own matching logic against real apm text —
# no pattern is duplicated here to drift out of sync.
SKIP_REASON=""
if ! command -v apm > /dev/null 2>&1 || ! command -v git > /dev/null 2>&1; then
SKIP_REASON="SKIP: apm and git are both required to verify the hook's phrase match against real \`apm outdated\` output — everything above ran, this axis did not"
echo " $SKIP_REASON"
else
PROBE="$(mktemp -d)"
trap 'rm -rf "$FAKE_BIN" "$WORK" "$PROBE"' EXIT
UPSTREAM="$PROBE/upstream.git"
git init -q "$UPSTREAM"
git -C "$UPSTREAM" -c user.email=probe@example.invalid -c user.name=probe \
commit -q --allow-empty -m one
LOCKED_SHA="$(git -C "$UPSTREAM" rev-parse HEAD)"
git -C "$UPSTREAM" -c user.email=probe@example.invalid -c user.name=probe \
commit -q --allow-empty -m two
BRANCH="$(git -C "$UPSTREAM" symbolic-ref --short HEAD)"
{
for repo in alpha beta; do
printf '[url "%s/upstream"]\n' "$PROBE"
printf '\tinsteadOf = git@apm-probe.invalid:probe/%s\n' "$repo"
printf '\tinsteadOf = https://apm-probe.invalid/probe/%s\n' "$repo"
printf '\tinsteadOf = ssh://git@apm-probe.invalid/probe/%s\n' "$repo"
done
} > "$PROBE/gitconfig"
mkdir -p "$PROBE/consumer" "$PROBE/home"
# $1 = how many stale dependencies to stage. Writes a lockfile and echoes what
# the real `apm outdated` printed for it.
real_apm_outdated() {
local want="$1" repo
{
echo "lockfile_version: '1'"
echo "generated_at: '2026-01-01T00:00:00+00:00'"
echo "apm_version: 0.0.0"
echo "dependencies:"
for repo in $( [[ "$want" == 1 ]] && echo alpha || echo alpha beta ); do
echo "- host: apm-probe.invalid"
echo " name: probe-$repo"
echo " package_type: apm_package"
echo " repo_url: probe/$repo"
echo " resolved_ref: $BRANCH"
echo " resolved_commit: $LOCKED_SHA"
echo " version: 1.0.0"
done
echo "deployments: []"
} > "$PROBE/consumer/apm.lock.yaml"
(
cd "$PROBE/consumer" &&
env HOME="$PROBE/home" \
GIT_CONFIG_GLOBAL="$PROBE/gitconfig" \
GIT_CONFIG_NOSYSTEM=1 \
GIT_TERMINAL_PROMPT=0 \
apm outdated 2>&1
)
}
# Replay genuine output through the hook. A harness that stages nothing would
# make every assertion below vacuously true, so the staged row is checked
# first and a failure to stage is a FAIL, not a quiet pass.
for want in 1 2; do
genuine="$(real_apm_outdated "$want" || true)"
if ! grep -q "outdated" <<< "$genuine"; then
fail "probe staged no outdated dependency against the real apm (harness broken, not the hook): $(tr '\n' ' ' <<< "$genuine" | cut -c1-160)"
continue
fi
printf '%s\n' "$genuine" > "$PROBE/genuine-$want.txt"
cat > "$FAKE_BIN/apm" << EOF
#!/usr/bin/env bash
pwd > "$WORK/apm-cwd"
case "\$1" in
outdated) cat "$PROBE/genuine-$want.txt"; exit 0 ;;
update) touch "$WORK/update-was-called"; exit 0 ;;
esac
exit 0
EOF
chmod +x "$FAKE_BIN/apm"
rm -f "$WORK/update-was-called"
out="$(run_hook)"
if [[ -n "$out" ]] && grep -q "$want package" <<< "$(json_field additionalContext 2>/dev/null <<< "$out")"; then
pass "detects staleness in real \`apm outdated\` output and counts $want package(s)"
else
fail "real apm reported $want outdated dependency/dependencies but the hook did not act on it — apm reworded its summary line. Real output: $(tr '\n' ' ' < "$PROBE/genuine-$want.txt" | tail -c 120)"
fi
done
fi
# ---------------------------------------------------------------------------
echo ""
echo "Results: $PASS passed, $FAIL failed"
[[ $FAIL -eq 0 ]] || exit 1
# A skip only after everything runnable has run and passed: the mocked axis is
# still worth executing on a machine without apm, but the suite must not read
# as green when the real-apm axis was not verified. run-tests.sh reports 77 as
# SKIPPED and, at pre-push (--strict), as a setup error naming this reason.
[[ -z "$SKIP_REASON" ]] || exit 77
exit 0
Reference in New Issue View Git Blame Copy Permalink