diff --git a/plugins/kyberforge/.apm/hooks/check-apm-current.sh b/plugins/kyberforge/.apm/hooks/check-apm-current.sh index d1c57d5..ba989fd 100755 --- a/plugins/kyberforge/.apm/hooks/check-apm-current.sh +++ b/plugins/kyberforge/.apm/hooks/check-apm-current.sh @@ -13,18 +13,36 @@ # Inert in any project that does not consume packages through apm. set -uo pipefail +# Anchor on the project root, not the session's cwd. Claude Code exports +# CLAUDE_PROJECT_DIR for SessionStart hooks; a session opened in a subdirectory +# would otherwise miss the lockfile, no-op silently, and — worse — run the apm +# calls below against that wrong directory. Fall back to the cwd when the +# variable is absent, which keeps the hook inert-but-harmless under a host that +# does not set it. +project_dir="${CLAUDE_PROJECT_DIR:-$PWD}" + # No lockfile means nothing was installed through apm here — e.g. a host that # installed this plugin natively. Say nothing and cost nothing. -[[ -f apm.lock.yaml ]] || exit 0 +[[ -f "$project_dir/apm.lock.yaml" ]] || exit 0 command -v apm > /dev/null 2>&1 || exit 0 +# Every apm call below must see the same directory the guard just checked — +# `apm outdated` and `apm update` both resolve the lockfile from the cwd. +cd "$project_dir" || exit 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. +# +# 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 -grep -q "outdated dependencies found" <<< "$outdated_output" || exit 0 +grep -qE 'outdated dependenc(y|ies) found' <<< "$outdated_output" || exit 0 -stale_count="$(grep -oE '[0-9]+ outdated dependencies found' <<< "$outdated_output" | grep -oE '^[0-9]+' || true)" +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 diff --git a/plugins/kyberforge/.apm/hooks/hooks.json b/plugins/kyberforge/.apm/hooks/hooks.json index e33213b..4d1c7c4 100644 --- a/plugins/kyberforge/.apm/hooks/hooks.json +++ b/plugins/kyberforge/.apm/hooks/hooks.json @@ -5,7 +5,7 @@ "hooks": [ { "command": "${CLAUDE_PLUGIN_ROOT}/.apm/hooks/check-apm-current.sh", - "timeout": 320, + "timeout": 380, "type": "command" } ], diff --git a/plugins/kyberforge/docs/hooks.md b/plugins/kyberforge/docs/hooks.md index 6b31c46..66ddd9a 100644 --- a/plugins/kyberforge/docs/hooks.md +++ b/plugins/kyberforge/docs/hooks.md @@ -83,9 +83,33 @@ Note that apm's **executable-trust gate is off** unless the consuming project's `check-apm-current.sh` keeps an apm-consumed install level with its remote: it runs `apm outdated`, and if anything is behind, runs `apm update --yes` and returns `reloadSkills: true` so the running -session picks up the redeployed content. It exits silently when there is no `apm.lock.yaml` in the -working directory, which makes it inert for any host that installed this plugin natively rather than -through apm. Rationale, measurements, and the failure modes are in ADR-0019. +session picks up the redeployed content. Rationale, measurements, and the failure modes are in +ADR-0019. + +**Where it looks for the lockfile.** The hook resolves a project directory as `${CLAUDE_PROJECT_DIR}` +when the host exports it (Claude Code does, for SessionStart hooks) and the current directory +otherwise, then exits silently unless that directory holds an `apm.lock.yaml` — which is what makes +it inert for any host that installed this plugin natively rather than through apm. Both `apm` +invocations run against the same resolved directory. The earlier spelling checked a bare +`apm.lock.yaml` against the session's cwd, so a session opened in a subdirectory of an +apm-consuming repo no-opped silently. Keep the cwd fallback: a host that sets no +`CLAUDE_PROJECT_DIR` must still get inert-but-harmless behaviour, not an unset-variable error. + +**The `timeout` in `hooks.json` must exceed the script's own budget.** The script spends at most +`timeout 60 apm outdated` plus `timeout 300 apm update`; the hook entry declares `timeout: 380`, the +sum plus a buffer. Set it lower and a slow remote gets the hook SIGKILLed mid-`apm update`, leaving a +partially redeployed `.claude/skills/` and emitting no notice — precisely the silent failure the hook +exists to prevent. `tests/test-apm-current-hook.sh` pins the relationship (host timeout > sum of the +script's internal timeouts) rather than the literal, so raising either side alone fails the suite. + +**Staleness is detected by matching apm's summary line, and both spellings count.** `apm outdated` +has no `--json` or otherwise machine-readable output (verified against apm 0.28.0), so the hook +greps its text. apm prints `1 outdated dependency found` in the singular when exactly one package is +behind and `N outdated dependencies found` otherwise; matching only the plural silently misses a +one-package drift. Because a mocked `apm` would keep a reworded release invisible, the test suite +stages a genuinely outdated dependency against the **real** `apm` — a local git repo reached through +`url..insteadOf` rewrites, so it needs no network — and replays that genuine output through the +hook. ## GitHub Copilot CLI diff --git a/plugins/kyberforge/hooks/hooks.json b/plugins/kyberforge/hooks/hooks.json index e33213b..4d1c7c4 100644 --- a/plugins/kyberforge/hooks/hooks.json +++ b/plugins/kyberforge/hooks/hooks.json @@ -5,7 +5,7 @@ "hooks": [ { "command": "${CLAUDE_PLUGIN_ROOT}/.apm/hooks/check-apm-current.sh", - "timeout": 320, + "timeout": 380, "type": "command" } ], diff --git a/tests/test-apm-current-hook.sh b/tests/test-apm-current-hook.sh index 3f66535..631270d 100755 --- a/tests/test-apm-current-hook.sh +++ b/tests/test-apm-current-hook.sh @@ -23,11 +23,14 @@ 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`. A sentinel file records whether update was actually invoked. +# `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 ;; @@ -37,7 +40,22 @@ EOF chmod +x "$FAKE_BIN/apm" } -run_hook() { (cd "$WORK" && PATH="$FAKE_BIN:$PATH" bash "$HOOK" 2>/dev/null); } +# 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"; } @@ -134,6 +152,54 @@ 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" +echo "$out" | json_field additionalContext | grep -q "6 package" \ + && 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 ---" @@ -153,7 +219,150 @@ matcher="$(python3 -c 'import json,sys; d=json.load(open(sys.argv[1])); print(d[ [[ "$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..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" ]] && echo "$out" | json_field additionalContext 2>/dev/null | grep -q "$want package"; 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 ]] +[[ $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