diff --git a/plugins/kyberforge/.apm/hooks/check-apm-current.sh b/plugins/kyberforge/.apm/hooks/check-apm-current.sh index 2200a73..bf4a15b 100755 --- a/plugins/kyberforge/.apm/hooks/check-apm-current.sh +++ b/plugins/kyberforge/.apm/hooks/check-apm-current.sh @@ -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" diff --git a/tests/test-apm-current-hook.sh b/tests/test-apm-current-hook.sh index 876333a..f456baa 100755 --- a/tests/test-apm-current-hook.sh +++ b/tests/test-apm-current-hook.sh @@ -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"