From 807caf22eec39c2b254171444f9a606cf7c11dc8 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Wed, 16 Sep 2026 11:24:32 +0000 Subject: [PATCH] fix(kyberforge): give branch-aware advice for the refreshed apm lock Why: the docs said to discard a refreshed apm.lock.yaml on a feature branch because the refresh records main's commit, but the branch's own lock records a (older) main commit too, and the SessionStart notice gave the same "commit or discard" advice on every branch. Implementation Notes: - check-apm-current.sh picks fixed advice by branch: commit or discard deliberately on the default branch (origin/HEAD, else main), discard and reinstall on a feature branch; the branch name is never interpolated. - README, AGENTS.md and ADR-0019 give the real reasons (no lock churn in the branch diff, deployed tree matches the committed lock), the cost (the session runs the older main) and that the next session start refreshes again. - ADR-0019's check-clean and stale-server claims restated to match apm's source. ADR: 0019 Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 2 +- README.md | 2 +- ...tart-hook-keeps-the-apm-install-current.md | 39 +++++++++----- .../.apm/hooks/check-apm-current.sh | 19 ++++++- tests/test-apm-current-hook.sh | 54 +++++++++++++++++++ 5 files changed, 101 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3aeb20d..fd7b3c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,7 +28,7 @@ Fall back to raw shell only when no skill covers it. ## Session rules - **Do not add repo-owned keys to `.claude/settings.json`.** apm treats it as its own deployed artifact and `apm audit --ci` replays the install and diffs, so anything apm would not have written is permanent drift that fails the `apm-audit-ci` pre-push hook. A hook you want here is authored in `plugins//.apm/hooks/` and deployed by apm, never hand-written into that file. The `SessionStart` entry already in it is exactly that: kyberforge authors it in `plugins/kyberforge/.apm/hooks/hooks.json` and apm merges it in, so it is apm's own output, it is what the replay expects, and it belongs in the commit — do not strip it (ADR-0019). Machine-specific settings go in the gitignored `.claude/settings.local.json`; shared enforcement goes in `.pre-commit-config.yaml`. -- **`apm.lock.yaml` turning up modified is expected, not a bug.** kyberforge's `SessionStart` hook keeps the install current on launch and rewrites the lock in the process (ADR-0019). On `main`, commit or discard it deliberately; on a feature branch, discard it (`git checkout -- apm.lock.yaml`, then `apm install`) — the refresh resolved against `main`, not the branch. +- **`apm.lock.yaml` turning up modified is expected, not a bug.** kyberforge's `SessionStart` hook keeps the install current on launch and rewrites the lock in the process (ADR-0019). On `main`, commit or discard it deliberately. On a feature branch, discard it (`git checkout -- apm.lock.yaml`, then `apm install`). This keeps unrelated lock churn out of the branch diff and keeps `apm pack --check-clean` consistent with the committed lock. The session then runs the older `main` that the lock records, which is accepted on a branch, and the next session start refreshes again. - **A `.apm/` edit is not live until it is on the remote's `main`.** The six dependencies resolve from the holocron remote, unpinned against the default branch, so pushing a feature branch does not deploy it (ADR-0019). `apm install` deploys from the lock; `apm update` is what re-resolves refs. - **No pre-push hook needs the network.** Root `apm.yml`'s marketplace has no remote package entries, so every hook resolves locally. - **This repo and Gitea are the only source of truth.** All project state, decisions, and working conventions live here. Do not use an external memory system for this project — cached state diverges from the repo and you get a split brain. Before answering any design or architecture question, check `docs/adr/` for an existing decision. diff --git a/README.md b/README.md index cf207d8..13d0159 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ pre-commit install -t pre-commit -t commit-msg -t pre-push ## Keeping the install current -The six dependencies in root `apm.yml` are unpinned against the default branch, so deployed skills go stale whenever anyone merges. kyberforge's `SessionStart` hook keeps the install current automatically on launch, rewriting `apm.lock.yaml` in the process — an unexplained modification to it after opening a session is expected, not a bug. On `main`, commit or discard it deliberately; on a feature branch, discard it (`git checkout -- apm.lock.yaml`, then `apm install`), because the refresh resolves against `main` and the lock now records `main`'s commit, not the branch's. Mechanism and rationale: `docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md`. +The six dependencies in root `apm.yml` are unpinned against the default branch, so deployed skills go stale whenever anyone merges. kyberforge's `SessionStart` hook keeps the install current automatically on launch, rewriting `apm.lock.yaml` in the process — an unexplained modification to it after opening a session is expected, not a bug. On `main`, commit or discard it deliberately. On a feature branch, discard it (`git checkout -- apm.lock.yaml`, then `apm install`). The committed lock records a `main` commit too, just an older one. Discarding keeps lock churn unrelated to the branch out of its diff, and keeps the deployed tree consistent with the committed lock that `apm pack --check-clean` reads. The trade-off: the session then runs the older `main` the lock records, which is accepted on a feature branch. The discard also lasts only until the next session start, when the hook finds the lock behind `main` and refreshes again. Mechanism and rationale: `docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md`. Note the difference between the two commands: diff --git a/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md b/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md index f9352d3..7f13d1d 100644 --- a/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md +++ b/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md @@ -152,18 +152,33 @@ Until then the repo has the mechanism in source and not in effect. > **Amendment (2026-09-16) — on a feature branch, the refresh installs `main`, not the branch.** > Recorded after it happened. The dependencies resolve against the remote default branch, so a > session opened on a branch that changes `plugins/` loads `main`'s content, refreshed or not — a -> branch's own `.apm/` edits are live only once they are on the remote's `main`. Two visible -> effects follow. Content the branch *removes* comes back in the deployed install: on -> `docs/simplification-audit` a refresh redeployed `main`'s `skill-audit` and `agent-audit` over -> the branch's merged `factory-audit`, and re-materialised `main`'s `plugins/bin/.mcp.json` into -> `apm_modules/`, so the gitignored root `.mcp.json` regained the `obsidian` server the branch -> deleted — invisible to `git status`. And the rewritten `apm.lock.yaml` records `main`'s commit, -> so on a branch it should be discarded (`git checkout -- apm.lock.yaml`, then `apm install` to -> bring the deployed tree back in line with the lock, or `apm pack --check-clean` refuses to run), -> not committed. Skipping the refresh off the default branch was considered and rejected: it would -> not make the branch live, only freeze the session on an older `main` — the silent staleness this -> ADR exists to prevent. Both effects end when the branch merges; apm removes a server that is no -> longer declared on its next update. +> branch's own `.apm/` edits are live only once they are on the remote's `main`. Content the +> branch *removes* comes back in the deployed install: on `docs/simplification-audit` a refresh +> redeployed `main`'s `skill-audit` and `agent-audit` over the branch's merged `factory-audit`, and +> re-materialised `main`'s `plugins/bin/.mcp.json` into `apm_modules/`, so the gitignored root +> `.mcp.json` regained the `obsidian` server the branch deleted — invisible to `git status`. +> Skipping the refresh off the default branch was considered and rejected: it would not make the +> branch live, only freeze the session on an older `main` — the silent staleness this ADR exists to +> prevent. The redeployed content goes away once the branch merges. For the server, the next +> `apm update` or `apm install` that resolves a tree no longer declaring it removes it from +> `.mcp.json`: both commands call `MCPIntegrator.remove_stale` for every server listed under the +> lock's `mcp_servers` that no dependency declares any more (`apm_cli/install/mcp/integration.py`). +> +> The rewritten `apm.lock.yaml` is a separate matter. It records `main`'s current tip, but the +> branch's committed lock records a `main` commit too, just an older one, so committing the +> rewrite would not swap the branch for `main`. On a feature branch, discard it anyway +> (`git checkout -- apm.lock.yaml`, then `apm install`), for two reasons. First, it keeps lock +> churn that has nothing to do with the branch out of the branch's diff. Second, it keeps the +> deployed tree consistent with the committed lock that the `apm-pack-check-clean` pre-push hook +> reads. In this repo `apm pack` builds a bundle from the lock before its `--check-clean` gate +> runs, and it stops with a build error ("deployed files are missing on disk -- run 'apm +> install'") when a file the lock lists is absent (`apm_cli/bundle/packer.py`, `pack_bundle`). +> A refresh leaves the tree in that state whenever the newer `main` dropped a file the older lock +> still lists. Under `--dry-run` it checks only that each file exists, not its content hash. The +> cost of discarding is real: the session then runs the older `main` that the lock records, which +> is the staleness the rejected skip would have caused. That cost is accepted on a feature branch, +> and it does not last. At the next session start the hook finds the restored lock behind `main` +> and refreshes again. **`.claude/settings.json` stops being `{"hooks": {}}`.** apm merges the hook into it and tracks ownership in a `.claude/apm-hooks.json` sidecar, with the script copied to diff --git a/plugins/kyberforge/.apm/hooks/check-apm-current.sh b/plugins/kyberforge/.apm/hooks/check-apm-current.sh index ba989fd..1475303 100755 --- a/plugins/kyberforge/.apm/hooks/check-apm-current.sh +++ b/plugins/kyberforge/.apm/hooks/check-apm-current.sh @@ -51,8 +51,25 @@ 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 +# selects between fixed strings and is never interpolated. Outside a git checkout, +# or on a detached HEAD, the neutral advice stands. +lock_advice="commit it or discard it deliberately." +current_branch="$(git symbolic-ref --short -q HEAD 2> /dev/null || true)" +if [[ -n "$current_branch" ]]; then + default_branch="$(git symbolic-ref --short -q refs/remotes/origin/HEAD 2> /dev/null || true)" + default_branch="${default_branch#origin/}" + if [[ "$current_branch" == "${default_branch:-main}" ]]; then + lock_advice="this is the default branch, so commit it or discard it deliberately." + else + lock_advice="this is a feature branch, so discard it: git checkout -- apm.lock.yaml && apm install" + fi +fi + if timeout 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 - commit it or discard it deliberately." + 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" fi diff --git a/tests/test-apm-current-hook.sh b/tests/test-apm-current-hook.sh index 8e1acb2..7f29cbb 100755 --- a/tests/test-apm-current-hook.sh +++ b/tests/test-apm-current-hook.sh @@ -21,6 +21,11 @@ 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 +# The hook asks git which branch it is on. Stop git's discovery at the temp +# root so a $TMPDIR that happens to sit inside a checkout cannot leak a branch +# into the fixtures that are meant to be outside one. +export GIT_CEILING_DIRECTORIES +GIT_CEILING_DIRECTORIES="$(dirname "$WORK")" # Mock `apm`. $1 chooses what `apm outdated` reports; $2 the exit code of # `apm update`. Sentinel files record whether update was actually invoked and @@ -124,6 +129,55 @@ else fail "emits valid JSON" fi +# --------------------------------------------------------------------------- +echo "" +echo "--- lock advice follows the branch ---" +# --------------------------------------------------------------------------- + +# ADR-0019: on the default branch the rewritten lock is a real update to commit or +# discard; on a feature branch it is unrelated churn to discard. Outside a git +# checkout (the fixture above) the neutral advice stands. +advice_of() { json_field additionalContext <<< "$1"; } +grep -q "commit it or discard it deliberately" <<< "$(advice_of "$out")" \ + && pass "gives neutral lock advice outside a git checkout" \ + || fail "outside a git checkout the advice should stay neutral" + +if command -v git > /dev/null 2>&1; then + REPO="$WORK/repo" + mkdir -p "$REPO" + git -C "$REPO" init -q -b main + git -C "$REPO" -c user.email=probe@example.invalid -c user.name=probe \ + commit -q --allow-empty -m init + touch "$REPO/apm.lock.yaml" + + out="$(run_hook_in "$REPO" "$REPO")" + grep -q "default branch, so commit it or discard it deliberately" <<< "$(advice_of "$out")" \ + && pass "on main, says to commit or discard the lock deliberately" \ + || fail "on main the advice should be commit-or-discard: $(advice_of "$out")" + + git -C "$REPO" checkout -q -b feature/x + out="$(run_hook_in "$REPO" "$REPO")" + advice="$(advice_of "$out")" + grep -qF "feature branch, so discard it: git checkout -- apm.lock.yaml && apm install" <<< "$advice" \ + && pass "on a feature branch, says to discard the lock and reinstall" \ + || fail "on a feature branch the advice should be discard-and-install: $advice" + grep -q "commit it" <<< "$advice" \ + && fail "on a feature branch the advice must not suggest committing the lock" \ + || pass "on a feature branch, does not suggest committing the lock" + echo "$out" | python3 -m json.tool > /dev/null 2>&1 \ + && pass "feature-branch notice is valid JSON" || fail "feature-branch notice broke the JSON" + + # A remote whose default branch is not `main` is honoured via origin/HEAD. + git -C "$REPO" update-ref refs/remotes/origin/feature/x HEAD + git -C "$REPO" symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/feature/x + out="$(run_hook_in "$REPO" "$REPO")" + grep -q "default branch, so commit it" <<< "$(advice_of "$out")" \ + && pass "reads the default branch from origin/HEAD when it is set" \ + || fail "should treat origin/HEAD's branch as the default: $(advice_of "$out")" +else + echo " (git not on PATH — branch-specific advice cases not run)" +fi + # --------------------------------------------------------------------------- echo "" echo "--- stale, refresh fails ---"