Files
holocron/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md
Defame1297 3a9d257225 fix(apm): commit the hook ownership sidecar so fresh installs stay idempotent
apm recognises its own settings.json hook entries only through the
.claude/apm-hooks.json sidecar. With the sidecar gitignored, apm install
in a fresh clone keeps the committed SessionStart entry as user-owned and
appends a duplicate, so apm audit --ci reports drift and the apm-audit-ci
pre-push hook fails. Reproduced on main and this branch with apm 0.28.0.

Commit the sidecar in apm's exact serialisation, exclude it from
pretty-format-json alongside settings.json, and record the correction
in ADR-0019.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-16 15:26:56 +00:00

233 lines
17 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# A SessionStart hook keeps the apm install current, replacing a git hook that never ran
ADR-0018 switched this repo to consuming its own plugins through `apm install`, with the six
packages declared as unpinned git refs against the holocron remote's default branch. That decision
left a hole it named but did not fill: the deployed content goes stale the moment anyone merges,
and nothing detects it.
**Status: accepted (2026-08-14).**
## Context
The pre-existing answer was `scripts/git-hooks/post-push`, which pulled the marketplace clone and
ran `claude plugin update kyberforge`. Issue #78 filed it as a bug — the hook updated `kyberforge`
but not `gitea`, so gitea skills stayed pinned at a pre-refactor version after #67 merged.
The issue's premise was wrong in a way nobody had noticed for six weeks. **Git has no client-side
`post-push` hook.** `githooks(5)` does not list one, and git 2.39.5 does not invoke one.
`scripts/install.sh` copies every file in `scripts/git-hooks/` into `.git/hooks/`, so
`.git/hooks/post-push` existed on disk and looked installed. It had never fired. The hook did not
skip `gitea`; it skipped everything. Both tests that appeared to cover it — `test-post-push.sh` and
`test-git-hooks-install.sh` — asserted only that the script behaved correctly when invoked directly
and that install.sh copied the file. Neither asserted that git ever runs it.
That also makes the original framing wrong. Refreshing on push assumes the person who pushes is the
person who goes stale, which is backwards: your install goes stale when *someone else* merges, and a
push of your own is neither necessary nor sufficient for it to have happened.
## Decision
A `SessionStart` hook, shipped in `plugins/kyberforge/.apm/hooks/`, checks whether the install is
behind and refreshes it in place.
`SessionStart` is the correct trigger because the thing that goes stale is the skill content a
*session* loads, and that is the moment the staleness does damage. It also enables two things a git
hook structurally cannot do: `additionalContext` puts the notice into the agent's context rather
than terminal scrollback nobody reads, and `reloadSkills: true` makes the host re-scan the skill
directories after the hook returns, so a refresh lands in the running session without a restart.
apm's own lifecycle events (`pre-/post-install`, `pre-/post-update`, `pre-/post-uninstall`) were
rejected: they fire around apm operations already chosen, so they can announce a refresh but never
detect that one is needed.
Three sub-decisions:
- **Refresh automatically rather than report.** The hook runs `apm update --yes` and asks for a skill
reload. The rejected alternative was to report and let a human run it. Auto-refresh costs a
rewritten `apm.lock.yaml` — a committed file — appearing as an unexplained modification in the
working tree, on any branch, at any time. The emitted notice says so explicitly for that reason.
- **`plugins/kyberforge/.apm/hooks/`, not `.claude/settings.json`.** ADR-0018 established that apm
owns `.claude/settings.json` and that any repo-authored key in it is permanent `apm audit --ci`
drift. A hook shipped in a package is written into that file by apm itself, so it is apm's output
and does not drift. `.claude/settings.local.json` also works but is gitignored and machine-local,
which fails the requirement that this travel with the repo.
- **`startup` matcher only.** `resume`, `clear`, `compact` and `fork` would re-run the check on every
compaction, and a compaction is not an event after which the remote can have moved.
The executable-trust gate is switched on at the same time. Root `apm.yml` gains an `executables:`
block allowing kyberforge's hooks and bin.
## Consequences
**The gate is off until something turns it on, and this repo had it off.** `apm approve --list`
reports `Executable-trust gate disabled -- all executables deploy` until an `executables:` block
exists in `apm.yml`. Any hook, bin, or MCP primitive a dependency shipped would have deployed with
no prompt and no record. The block added here closes that for this repo; every other apm project on
this machine still has it open.
**The allow key is version-pinned, and that is a live failure mode.** apm writes
`kyberforge#1.5.0`, not `kyberforge` — and the release that ships this hook proved the point
immediately, since bumping kyberforge to 1.5.0 required editing the key in the same commit. A
kyberforge version bump makes the entry stop matching, the
gate blocks the hook, and the install silently stops refreshing — the exact failure this ADR exists
to end, reintroduced through the mechanism meant to secure it.
Matching is an exact dictionary lookup on the composed `name#version` string
(`apm_cli/security/executables.py`, `is_package_approved`), so there is no wildcard or
version-less key that would sidestep this — the key has to be edited on every bump, and the
question is only what catches a missed edit. A comment in the `executables:` block is not enough:
this repo gates generated-content drift, marketplace mirror drift and vale style drift
deterministically, and a silent-staleness failure is strictly worse than any of them. So
`scripts/check-executables-allow-sync.sh` runs at pre-push, parsing `version:` out of
`plugins/kyberforge/apm.yml` and asserting root `apm.yml` carries the matching
`kyberforge#<version>` key. The comment stays as the human-facing pointer; the hook is what
actually holds. It parses with PyYAML where importable and falls back to a two-shape scan
otherwise, so a missing pip package cannot become the thing that blocks every push.
**Trust is keyed on the version, not on the content.** `kyberforge#1.5.0` approves whatever
`check-apm-current.sh` contains at the moment it is fetched, not the bytes that were reviewed when
the key was written. Because the dependency is unpinned against the default branch and the hook
runs `apm update --yes` unattended, an edit to that script landing on `main` deploys and executes
on every contributor's machine at their next session start, with no second approval prompt and no
diff shown. The trust gate constrains *which package* may ship an executable; it does not constrain
what that executable does between version bumps. That is an accepted property of this design rather
than an oversight — the remote is self-hosted, push access to `main` is already sufficient to
change any skill body an agent will follow — but it is the reason the gate should not be read as a
supply-chain control. Pinning each dependency to a `ref:` is what would make it one, and ADR-0018
defers that until per-package release tags exist.
**A referenced hook script must be addressed at its `.apm/` path.** apm resolves
`${CLAUDE_PLUGIN_ROOT}/...` against the installed package root, and `apm pack` keeps only `*.json`
from `.apm/hooks/` when it builds the flat mirror. So `${CLAUDE_PLUGIN_ROOT}/hooks/check-apm-current.sh`
resolves to the mirror, where the script does not exist — verified, apm reports
`Hook script not found` and deploys a hook pointing at nothing. The working reference is
`${CLAUDE_PLUGIN_ROOT}/.apm/hooks/check-apm-current.sh`. The script cannot simply be placed in
`plugins/kyberforge/hooks/` either: that directory is `rm -rf`'d by every content sync (ADR-0017).
A test pins the reference.
> **Amendment (2026-09-14) — the mirror half of that reasoning is gone; the conclusion is not.**
> ADR-0024 deleted the flat mirror and `scripts/sync-plugin-content.sh`, so neither the `apm pack`
> mirror-filtering behaviour nor the `rm -rf` content sync described above still happens. The
> reference must stay exactly as written, for the reason that survives independently: `.apm/` is the
> sole hand-edited authoring source (ADR-0015), it is what ships in the installed package root, and
> `plugins/kyberforge/hooks/` no longer exists at all — so `${CLAUDE_PLUGIN_ROOT}/hooks/...` still
> names a path with nothing at it, now because the directory is gone rather than because a sync
> emptied it. `tests/test-apm-current-hook.sh` still pins the literal string.
**Session startup gets slower when the install is stale.** Measured: ~0.7 s for the `apm outdated`
check when everything is current, ~10.4 s when six packages are behind and the refresh runs. The
hook declares `timeout: 380` to cover a cold multi-package fetch. That number is not free-standing:
the script imposes its own `timeout 60` on `apm outdated` and `timeout 300` on `apm update`, so the
host-side timeout has to exceed their sum or the host kills the hook mid-update and leaves
`.claude/skills/` half-deployed with no notice emitted. An earlier revision declared `320`, which
was below the 360 s the script can legitimately take. A test asserts the invariant rather than the
literal — it parses every `timeout N` out of the script, sums them, and requires the `hooks.json`
value to be larger — so changing either side without the other fails the suite.
> **Amendment (2026-09-16) — the refresh is slower than first measured, still inside the budget.**
> Re-measured: ~24–26 s for the same six-behind refresh, warm, on a LAN remote — well inside the
> 380 s above.
**Reading a human-readable CLI for a control decision cost a silent failure, again.** `apm outdated`
has no `--json` or other machine-readable flag (confirmed against 0.28.0), so the hook must match
its prose. The first attempt matched `outdated dependencies found` — plural only. apm emits
`1 outdated dependency found` in the singular when exactly one package is behind
(`apm_cli/commands/outdated.py`), so a single stale package was invisible: the hook exited 0
silently and no refresh ran. With six packages merging independently, one-behind is the ordinary
case rather than an edge, which means the mechanism failed most often in exactly the situation it
exists for. The match is now `outdated dependenc(y|ies) found`.
The deeper lesson is the one `post-push` already taught and this repeated: every assertion about the
hook mocked `apm`, so the suite was green while the hook could not detect the common case. Mocks
verify the code against its author's belief about the interface, never the interface. The suite now
carries one probe that stages a genuinely outdated dependency against a local git remote — offline,
via `url.<path>.insteadOf`, so the twelve-hooks-pass-under-`unshare -rn` property survives — runs
the real `apm outdated`, and replays its genuine output through the real hook. Reverting the grep
to plural-only fails it.
**The hook cannot install itself.** Dependencies resolve from the remote, so the hook does not
deploy until this change is merged and `apm update` has run once against the new default branch.
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`. 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
`.claude/hooks/<pkg>/`. The sidecar and the script directory are gitignored install output; the
settings file remains committed, now with apm-generated content in it. ADR-0018's statement that the
committed content is exactly `{"hooks": {}}` is superseded on that point only — the rule it was
protecting, that nothing repo-authored goes in that file, is unchanged.
> **Correction (2026-09-16) — the sidecar is committed, not ignored.** apm keeps no ownership marker
> inside `settings.json`; it recognises its own entries by matching them against
> `.claude/apm-hooks.json`, then replaces them. With the sidecar gitignored, a fresh clone holds the
> committed `SessionStart` entry but no sidecar, so `apm install` treats the entry as user-owned,
> keeps it, and adds its own identical copy. `apm audit --ci` then reports `settings.json` drift and
> the `apm-audit-ci` pre-push hook fails. Reproduced on `main` (`a712f2c`) and on this branch with
> apm 0.28.0; committing the sidecar makes the install idempotent and the audit pass. The sidecar
> is apm output like the settings entry it describes, so it is committed for the same reason and
> changes only when the owning package is renamed or moved. `.claude/hooks/` stays ignored.
**Native consumers are protected by a guard, not by the gate.** A host installing holocron through
`claude plugin install` auto-discovers `hooks/hooks.json` and does not consult apm's trust gate at
all. The script therefore exits silently when there is no `apm.lock.yaml` in the working directory,
which is what makes it inert in a repo that does not consume packages through apm. Copilot CLI sees
no hook at all, for the reasons already documented in `plugins/kyberforge/docs/hooks.md`.
> **Amendment (2026-09-14) — both halves of that paragraph are gone; the guard is kept on other
> grounds.** There is no `hooks/hooks.json` to auto-discover: `718c79a` deleted
> `plugins/kyberforge/hooks/` with the rest of the flat mirror, and the only hooks manifest left in
> the tree is `plugins/kyberforge/.apm/hooks/hooks.json`, which apm reads and a convention scan of
> the package root never sees. And there are no native consumers to protect: ADR-0024 ended
> `claude plugin install` support outright. The `apm.lock.yaml` guard itself stays, for the reason
> that survives both — kyberforge ships to any apm consumer, and in a working directory with no
> lockfile there is nothing for `apm update` to refresh, so exiting silently is the correct
> behaviour rather than a defensive measure aimed at a second installer. The Copilot CLI sentence is
> unaffected.
**`scripts/git-hooks/` is now empty.** `post-push` and `test-post-push.sh` are deleted.
`install.sh`'s copy block is generic and is kept; `test-git-hooks-install.sh` now synthesizes its
own fixture hook instead of depending on a real one existing, so the mechanism stays tested and can
be used again if a hook git actually invokes is ever wanted.
## Alternatives considered
- **A `post-merge` git hook.** Real, unlike `post-push`, and verified to fire on both a
fast-forward `git pull` and a `git pull --rebase`. Rejected as the primary mechanism because a
pull is the wrong signal, and because it cannot reload skills in a running session. It remains
the only option for a project that consumes apm packages without a Claude-family host.
- **Reporting instead of refreshing.** See the sub-decision above.
- **A seventh plugin holding only this hook**, to avoid shipping it to external kyberforge
consumers. Rejected as disproportionate: the `apm.lock.yaml` guard already makes the hook inert
for anyone not consuming through apm, and a package exists to be maintained, versioned, and
registered in the marketplace.