fix(kyberforge): make the apm currency hook portable and bounded
Why: stock macOS has no timeout(1), so the hook exited silently and never checked the install, the silent staleness ADR-0019 exists to prevent. - fall back to gtimeout, else emit a notice instead of running apm unbounded - kill after a 5s grace; worst case 370s stays under the 380s host limit - export GIT_TERMINAL_PROMPT=0 so a credential prompt cannot hang startup - serialise concurrent refreshes with flock when available Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkT7RSDwDbmrM9T34b6sTi
This commit is contained in:
@@ -38,27 +38,47 @@ command -v apm > /dev/null 2>&1 || exit 0
|
||||
# `apm outdated` and `apm update` both resolve the lockfile from the cwd.
|
||||
cd "$project_dir" || exit 0
|
||||
|
||||
# Only ever emit fixed text plus a digit-checked count — never interpolate
|
||||
# command output into the JSON, which would need escaping this cannot do safely.
|
||||
emit() {
|
||||
printf '{"hookSpecificOutput":{"hookEventName":"SessionStart","reloadSkills":%s,"additionalContext":"%s"}}\n' "$1" "$2"
|
||||
}
|
||||
|
||||
# Every apm call is time-boxed, so timeout(1) is a hard requirement. It is GNU
|
||||
# coreutils: stock macOS has none, and Homebrew's coreutils installs it as
|
||||
# `gtimeout`. Calling a missing binary would exit 127, which the `|| exit 0`
|
||||
# below swallows — the hook would silently never work. Say so once instead, and
|
||||
# do not run apm unbounded.
|
||||
if command -v timeout > /dev/null 2>&1; then
|
||||
timeout_bin="timeout"
|
||||
elif command -v gtimeout > /dev/null 2>&1; then
|
||||
timeout_bin="gtimeout"
|
||||
else
|
||||
emit false "The apm install currency check did not run: neither timeout nor gtimeout (GNU coreutils) is on PATH, and this hook will not run apm without a time limit. Install coreutils (macOS: brew install coreutils) or run apm outdated by hand."
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# apm drives git for every remote ref. A remote that wants credentials must fail
|
||||
# fast, not block on a terminal prompt nobody can see until the timeout fires.
|
||||
export GIT_TERMINAL_PROMPT=0
|
||||
|
||||
# `apm outdated` exits 0 whether or not anything is stale, so the answer has to
|
||||
# come from its output. ~0.7s against six remote refs; a hung remote must not
|
||||
# hold the session open.
|
||||
# hold the session open. -k sends SIGKILL a grace period after the SIGTERM, so a
|
||||
# child that ignores TERM cannot outlive its budget; tests/test-apm-current-hook.sh
|
||||
# sums every limit plus its grace against the hooks.json timeout.
|
||||
#
|
||||
# There is no --json/machine-readable flag on `apm outdated` (verified against
|
||||
# apm 0.28.0), so the phrase match is forced rather than chosen. Note the
|
||||
# singular: apm prints "1 outdated dependency found" when exactly one package is
|
||||
# behind, so matching only "dependencies" would silently miss a one-package
|
||||
# drift. tests/test-apm-current-hook.sh pins both spellings against the real apm.
|
||||
outdated_output="$(timeout 60 apm outdated 2>&1)" || exit 0
|
||||
outdated_output="$("$timeout_bin" -k 5 60 apm outdated 2>&1)" || exit 0
|
||||
grep -qE 'outdated dependenc(y|ies) found' <<< "$outdated_output" || exit 0
|
||||
|
||||
stale_count="$(grep -oE '[0-9]+ outdated dependenc(y|ies) found' <<< "$outdated_output" | grep -oE '^[0-9]+' || true)"
|
||||
[[ "$stale_count" =~ ^[0-9]+$ ]] || stale_count="some"
|
||||
|
||||
# Only ever emit fixed text plus a digit-checked count — never interpolate
|
||||
# command output into the JSON, which would need escaping this cannot do safely.
|
||||
emit() {
|
||||
printf '{"hookSpecificOutput":{"hookEventName":"SessionStart","reloadSkills":%s,"additionalContext":"%s"}}\n' "$1" "$2"
|
||||
}
|
||||
|
||||
# What to do with the rewritten lock depends on the branch (ADR-0019): on the
|
||||
# default branch it is a real update to commit or discard; on a feature branch it
|
||||
# is churn unrelated to the branch and should be discarded. The branch name only
|
||||
@@ -85,7 +105,23 @@ if [[ -n "$current_branch" && -n "$default_branch" ]]; then
|
||||
fi
|
||||
fi
|
||||
|
||||
if timeout 300 apm update --yes > /dev/null 2>&1; then
|
||||
# Two sessions started together would both run `apm update --yes` over the same
|
||||
# tree. Serialise on a lock under apm_modules/: `apm install` itself adds that
|
||||
# directory to .gitignore, so the lock never shows up as a working-tree change,
|
||||
# and apm only ever removes package directories inside it, never the directory
|
||||
# (or this file) itself. The loser does not wait — the winner's refresh is the
|
||||
# one it wanted — and says so. flock(1) is util-linux, absent on stock macOS,
|
||||
# and there the refresh runs unserialised, as it did before the lock existed; so
|
||||
# does a checkout with no apm_modules/ yet, rather than creating it.
|
||||
if command -v flock > /dev/null 2>&1 && [[ -d apm_modules ]] \
|
||||
&& { exec 9> apm_modules/.kyberforge-apm-update.lock; } 2> /dev/null; then
|
||||
if ! flock -n 9; then
|
||||
emit false "apm install is ${stale_count} package(s) behind the remote default branch, and another session is refreshing it right now, so this session skipped its own refresh. Skills and agents loaded in this session may be stale; if they are, restart the session once that refresh has finished."
|
||||
exit 0
|
||||
fi
|
||||
fi
|
||||
|
||||
if "$timeout_bin" -k 5 300 apm update --yes > /dev/null 2>&1; then
|
||||
emit true "apm install was ${stale_count} package(s) behind the remote default branch and has been refreshed automatically; skills and agents were redeployed and re-scanned. apm.lock.yaml has been rewritten and is now a modified file in the working tree - ${lock_advice}"
|
||||
else
|
||||
emit false "apm install is ${stale_count} package(s) behind the remote default branch and the automatic refresh failed. Deployed skills and agents may be stale. Run: apm update --yes"
|
||||
|
||||
@@ -36,6 +36,7 @@ make_apm() {
|
||||
cat > "$FAKE_BIN/apm" << EOF
|
||||
#!/usr/bin/env bash
|
||||
pwd > "$WORK/apm-cwd"
|
||||
echo "\${GIT_TERMINAL_PROMPT-unset}" > "$WORK/gtp-\$1"
|
||||
case "\$1" in
|
||||
outdated) echo "$outdated_line"; exit 0 ;;
|
||||
update) touch "$WORK/update-was-called"; exit $update_exit ;;
|
||||
@@ -297,14 +298,147 @@ out="$(run_hook_in "$WORK" "")"; rc=$?
|
||||
|
||||
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"
|
||||
[[ $rc -eq 0 ]] && pass "exits 0 when the project dir has no lockfile" \
|
||||
|| fail "should exit 0 when the project dir has no lockfile"
|
||||
[[ -z "$out" ]] && pass "stays silent when the project dir has no lockfile" \
|
||||
|| fail "should stay silent when the project dir has no lockfile"
|
||||
[[ ! -f "$WORK/update-was-called" ]] \
|
||||
&& pass "does not run apm update when there is no lockfile anywhere" \
|
||||
&& pass "does not run apm update when the project dir has no lockfile" \
|
||||
|| fail "must not touch a project that does not use apm"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
echo ""
|
||||
echo "--- git never prompts ---"
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# A remote that wants credentials must fail fast inside apm's git calls, not
|
||||
# block on a terminal prompt until the timeout fires. Cleared on the way in so
|
||||
# the assertion cannot pass on an inherited value.
|
||||
make_apm "[!] 6 outdated dependencies found" 0
|
||||
rm -f "$WORK/gtp-outdated" "$WORK/gtp-update"
|
||||
(cd "$WORK" && env -u GIT_TERMINAL_PROMPT CLAUDE_PROJECT_DIR="$WORK" PATH="$FAKE_BIN:$PATH" bash "$HOOK" > /dev/null 2>&1)
|
||||
[[ "$(cat "$WORK/gtp-outdated" 2>/dev/null)" == "0" ]] \
|
||||
&& pass "apm outdated runs with GIT_TERMINAL_PROMPT=0" \
|
||||
|| fail "apm outdated saw GIT_TERMINAL_PROMPT='$(cat "$WORK/gtp-outdated" 2>/dev/null)' — must be 0"
|
||||
[[ "$(cat "$WORK/gtp-update" 2>/dev/null)" == "0" ]] \
|
||||
&& pass "apm update runs with GIT_TERMINAL_PROMPT=0" \
|
||||
|| fail "apm update saw GIT_TERMINAL_PROMPT='$(cat "$WORK/gtp-update" 2>/dev/null)' — must be 0"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
echo ""
|
||||
echo "--- timeout(1) resolution ---"
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# Stock macOS has no timeout(1); Homebrew coreutils ships it as gtimeout. A PATH
|
||||
# that holds only what the hook and the mock need — and no timeout — stands in
|
||||
# for that host. bash is invoked by absolute path; `env` and `bash` are still
|
||||
# linked in because the mock's shebang resolves them through PATH.
|
||||
REAL_TIMEOUT="$(command -v timeout || true)"
|
||||
BASH_BIN="$(command -v bash)"
|
||||
SANDBOX_BIN="$(mktemp -d)"
|
||||
GT_BIN="$(mktemp -d)"
|
||||
trap 'rm -rf "$FAKE_BIN" "$WORK" "$SANDBOX_BIN" "$GT_BIN" "${PROBE:-}"' EXIT
|
||||
for tool in bash env grep touch git; do
|
||||
src="$(command -v "$tool" || true)"
|
||||
[[ -n "$src" ]] && ln -s "$src" "$SANDBOX_BIN/$tool"
|
||||
done
|
||||
|
||||
make_apm "[!] 6 outdated dependencies found" 0
|
||||
rm -f "$WORK/update-was-called" "$WORK/apm-cwd"
|
||||
out="$( (cd "$WORK" && env CLAUDE_PROJECT_DIR="$WORK" PATH="$FAKE_BIN:$SANDBOX_BIN" "$BASH_BIN" "$HOOK" 2>/dev/null) )"; rc=$?
|
||||
[[ $rc -eq 0 ]] && pass "exits 0 when neither timeout nor gtimeout is on PATH" \
|
||||
|| fail "exited $rc with no timeout binary — must exit 0"
|
||||
if echo "$out" | python3 -m json.tool > /dev/null 2>&1; then
|
||||
pass "emits valid JSON when timeout(1) is missing"
|
||||
[[ "$(echo "$out" | json_field reloadSkills)" == "False" ]] \
|
||||
&& pass "does not ask for a skill reload when timeout(1) is missing" || fail "reloadSkills should be false"
|
||||
grep -q "neither timeout nor gtimeout" <<< "$(json_field additionalContext <<< "$out")" \
|
||||
&& pass "says timeout(1) is missing instead of failing silently" \
|
||||
|| fail "the notice should name the missing timeout binary"
|
||||
else
|
||||
fail "emits valid JSON when timeout(1) is missing: $out"
|
||||
fi
|
||||
[[ ! -f "$WORK/apm-cwd" ]] && pass "runs no apm command without a time limit" \
|
||||
|| fail "ran apm unbounded with no timeout binary"
|
||||
|
||||
if [[ -n "$REAL_TIMEOUT" ]]; then
|
||||
cat > "$GT_BIN/gtimeout" << EOF
|
||||
#!/usr/bin/env bash
|
||||
touch "$WORK/gtimeout-was-called"
|
||||
exec "$REAL_TIMEOUT" "\$@"
|
||||
EOF
|
||||
chmod +x "$GT_BIN/gtimeout"
|
||||
|
||||
rm -f "$WORK/update-was-called" "$WORK/gtimeout-was-called"
|
||||
out="$( (cd "$WORK" && env CLAUDE_PROJECT_DIR="$WORK" PATH="$FAKE_BIN:$GT_BIN:$SANDBOX_BIN" "$BASH_BIN" "$HOOK" 2>/dev/null) )"
|
||||
[[ -f "$WORK/gtimeout-was-called" ]] && pass "falls back to gtimeout when timeout is absent" \
|
||||
|| fail "should use gtimeout when timeout is not on PATH"
|
||||
[[ -f "$WORK/update-was-called" && "$(echo "$out" | json_field reloadSkills 2>/dev/null)" == "True" ]] \
|
||||
&& pass "refreshes normally through gtimeout" || fail "the gtimeout path should refresh and reload: $out"
|
||||
|
||||
rm -f "$WORK/gtimeout-was-called"
|
||||
(cd "$WORK" && env CLAUDE_PROJECT_DIR="$WORK" PATH="$FAKE_BIN:$GT_BIN:$(dirname "$REAL_TIMEOUT"):$SANDBOX_BIN" "$BASH_BIN" "$HOOK" > /dev/null 2>&1)
|
||||
[[ ! -f "$WORK/gtimeout-was-called" ]] && pass "prefers timeout over gtimeout when both exist" \
|
||||
|| fail "used gtimeout although timeout is on PATH"
|
||||
else
|
||||
echo " (timeout not on PATH — gtimeout fallback cases not run)"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
echo ""
|
||||
echo "--- concurrent sessions do not both update ---"
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
if command -v flock > /dev/null 2>&1; then
|
||||
LOCK_PROJECT="$WORK/lock-project"
|
||||
mkdir -p "$LOCK_PROJECT/apm_modules"
|
||||
touch "$LOCK_PROJECT/apm.lock.yaml"
|
||||
LOCKFILE="$LOCK_PROJECT/apm_modules/.kyberforge-apm-update.lock"
|
||||
make_apm "[!] 6 outdated dependencies found" 0
|
||||
|
||||
# Lock free: the refresh runs and the lock lands under the gitignored
|
||||
# apm_modules/, never in the project root where git would see it.
|
||||
rm -f "$WORK/update-was-called"
|
||||
out="$(run_hook_in "$LOCK_PROJECT" "$LOCK_PROJECT")"
|
||||
[[ -f "$WORK/update-was-called" ]] && pass "updates when the lock is free" \
|
||||
|| fail "should update when no other session holds the lock"
|
||||
[[ -f "$LOCKFILE" ]] && pass "takes its lock under apm_modules/" \
|
||||
|| fail "expected the lock at apm_modules/.kyberforge-apm-update.lock"
|
||||
|
||||
# Lock held by another session: this one must not update, must still exit 0
|
||||
# with valid JSON, and must say why nothing was refreshed.
|
||||
exec 8> "$LOCKFILE"
|
||||
flock 8
|
||||
rm -f "$WORK/update-was-called"
|
||||
out="$(run_hook_in "$LOCK_PROJECT" "$LOCK_PROJECT" 8>&-)"; rc=$?
|
||||
exec 8>&-
|
||||
[[ $rc -eq 0 ]] && pass "exits 0 when another session holds the update lock" \
|
||||
|| fail "exited $rc with the lock held — must exit 0"
|
||||
[[ ! -f "$WORK/update-was-called" ]] && pass "skips apm update when another session holds the lock" \
|
||||
|| fail "ran apm update while another session held the lock"
|
||||
if echo "$out" | python3 -m json.tool > /dev/null 2>&1; then
|
||||
[[ "$(echo "$out" | json_field reloadSkills)" == "False" ]] \
|
||||
&& pass "does not ask for a skill reload when it skipped the refresh" || fail "reloadSkills should be false"
|
||||
grep -q "another session is refreshing it" <<< "$(json_field additionalContext <<< "$out")" \
|
||||
&& pass "says another session is refreshing" || fail "the notice should say another session holds the refresh"
|
||||
else
|
||||
fail "emits valid JSON when the lock is held: $out"
|
||||
fi
|
||||
|
||||
# No apm_modules/ yet (fresh clone before install): proceed unserialised
|
||||
# rather than create the directory.
|
||||
NO_MODULES="$WORK/no-modules"
|
||||
mkdir -p "$NO_MODULES"
|
||||
touch "$NO_MODULES/apm.lock.yaml"
|
||||
rm -f "$WORK/update-was-called"
|
||||
run_hook_in "$NO_MODULES" "$NO_MODULES" > /dev/null
|
||||
[[ -f "$WORK/update-was-called" && ! -e "$NO_MODULES/apm_modules" ]] \
|
||||
&& pass "without apm_modules/, updates unserialised and creates nothing" \
|
||||
|| fail "without apm_modules/ the hook should update and not create the directory"
|
||||
else
|
||||
echo " (flock not on PATH — lock cases not run)"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
echo ""
|
||||
echo "--- hooks.json wiring ---"
|
||||
@@ -333,15 +467,27 @@ matcher="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d[
|
||||
# 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.
|
||||
# Both spellings count, comments included: `-k K N` (the binary is resolved at
|
||||
# run time, so the call reads `"$timeout_bin" -k 5 60 …`) contributes the limit N
|
||||
# plus the kill-after grace K, since SIGKILL lands only K seconds after N; a
|
||||
# bare `timeout N` contributes N. A stray match 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))
|
||||
while read -r k n; do
|
||||
[[ -n "$k" ]] || continue
|
||||
script_budget=$((script_budget + k + ${n:-0}))
|
||||
timeout_count=$((timeout_count + 1))
|
||||
done < <(grep -oE '\btimeout [0-9]+\b' "$HOOK" | grep -oE '[0-9]+')
|
||||
done < <(
|
||||
grep -oE -- '-k [0-9]+ [0-9]+\b' "$HOOK" | grep -oE '[0-9]+ [0-9]+' || true
|
||||
grep -oE '\btimeout [0-9]+\b' "$HOOK" | grep -oE '[0-9]+' || true
|
||||
)
|
||||
|
||||
# Without -k a child that ignores SIGTERM outlives its limit and the budget
|
||||
# above is fiction. Every time-boxed call must carry the grace.
|
||||
unkilled="$(grep -E '"\$timeout_bin" ' "$HOOK" | grep -vE -- '-k [0-9]+ [0-9]+' || true)"
|
||||
[[ -z "$unkilled" ]] && pass "every time-boxed apm call carries a -k kill-after grace" \
|
||||
|| fail "a time-boxed call has no -k grace: $unkilled"
|
||||
|
||||
hook_timeout="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d["hooks"]["SessionStart"][0]["hooks"][0]["timeout"])' "$HOOKS_JSON")"
|
||||
|
||||
@@ -381,7 +527,7 @@ if ! command -v apm > /dev/null 2>&1 || ! command -v git > /dev/null 2>&1; then
|
||||
echo " $SKIP_REASON"
|
||||
else
|
||||
PROBE="$(mktemp -d)"
|
||||
trap 'rm -rf "$FAKE_BIN" "$WORK" "$PROBE"' EXIT
|
||||
trap 'rm -rf "$FAKE_BIN" "$WORK" "$SANDBOX_BIN" "$GT_BIN" "$PROBE"' EXIT
|
||||
|
||||
UPSTREAM="$PROBE/upstream.git"
|
||||
git init -q "$UPSTREAM"
|
||||
|
||||
Reference in New Issue
Block a user