refactor!: carry out the simplification audit across gates, tests, plugins and docs #135

Merged
Defame1297 merged 85 commits from docs/simplification-audit into main 2026-09-20 19:14:03 +00:00
5 changed files with 110 additions and 62 deletions
Showing only changes of commit dd0b9233e6 - Show all commits

View File

@@ -28,8 +28,8 @@ 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/<name>/.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). Commit or discard it deliberately.
- **A `.apm/` edit is not live in this session until it is pushed.** The six dependencies resolve from the holocron remote, unpinned against the default branch. `apm install` deploys from the lock; `apm update` is what re-resolves refs.
- **`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.
- **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.

View File

@@ -31,7 +31,7 @@ Install all of these before setting up. Each one is a hard dependency of a git h
| Tool | Why | Install |
| --- | --- | --- |
| `apm` CLI | Two pre-push hooks shell out to it (`apm-audit-ci` and `apm-pack-check-clean`) | The `apm-install` skill, or `curl -sSL https://aka.ms/apm-unix \| sh`. Verify with `apm --version` |
| `python3` + PyYAML | Required by `scripts/skill-size-check.sh` (the `skill-size-check` pre-commit hook), which reads folded YAML frontmatter | `python3` is usually present — pre-commit is itself a Python application. `pip install pyyaml` if the hook reports PyYAML missing |
| `python3` + PyYAML | Required by `scripts/skill-size-check.sh` (the `skill-size-check` pre-commit hook) and `scripts/check-skill-version-bump.sh` (the `check-skill-version-bump` pre-push hook), which both parse YAML frontmatter | `python3` is usually present — pre-commit is itself a Python application. `pip install pyyaml` if the hook reports PyYAML missing |
| `vale` | Required by the `vale-audit-prefilter-skill` / `-agent` pre-commit hooks, and by the `test-vale-wrap.sh` / `test-vale-hooks-consumer.sh` suites that `run-tests --strict` runs at pre-push | `brew install vale` (macOS), `snap install vale` (Linux), `choco install vale` (Windows), or https://vale.sh/docs/vale-cli/installation/ |
| `claude` CLI | Required by the `validate-marketplace` pre-push hook | Claude Code |
@@ -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; commit or discard it deliberately. 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`), 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`.
Note the difference between the two commands:

View File

@@ -115,9 +115,8 @@ A test pins the reference.
> 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
(re-measured 2026-09-16: ~24–26 s for the same six-behind refresh, warm, on a LAN remote — still
well inside the budget below). The hook declares `timeout: 380` to cover a cold multi-package fetch. That number is not free-standing:
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
@@ -125,6 +124,10 @@ was below the 360 s the script can legitimately take. A test asserts the invaria
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
@@ -146,20 +149,21 @@ to plural-only fails it.
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.
**On a feature branch, the refresh installs `main`, not the branch.** Added 2026-09-16, 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 — the branch's own edits
are never live until merged. 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.
> **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.
**`.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

View File

@@ -77,38 +77,49 @@ presence is no longer in question. `.pre-commit-config.yaml`'s `skill-frontmatte
to require the field, closing the gap #113 and #118 both named in the same audit pass: a stated rule
with nothing enforcing it drifts the same way an unstated one does.
## Amendment (2026-09-16) — the bump is enforced at push, not only required to exist
## Amendment (2026-09-16): the bump is enforced at push, not only required to exist
**Context.** Making the field mandatory did not make it move. The only thing that bumped it was
`skill-author` Step 4, so every hand edit and every trim pass skipped the bump: on
`docs/simplification-audit`, 17 of the 40 skill directories that changed against `main` carried
the same `metadata.version` as `main`, and `gitea` alone sat at six different values. Both
validators checked presence and semver shape, never movement, so the field could not answer the
question this ADR gives it — "did this change since I last read it". (Simplification audit
finding 33.)
Making the field mandatory did not make it move. The only thing that bumped it was `skill-author`
Step 4, so every hand edit and every trim pass skipped the bump: on `docs/simplification-audit`, 17
of the 40 skill directories that changed against `main` carried the same `metadata.version` as
`main`, and `gitea` alone sat at six different values. Both validators checked presence and semver
shape, never movement, so the field could not answer the question this ADR gives it — "did this
change since I last read it". (Simplification audit finding 33.)
**Decision.** `scripts/check-skill-version-bump.sh` runs as a pre-push hook on every push. For each
skill directory under `plugins/*/.apm/skills/` that differs between the pushed ref and its
merge-base with `main`, ignoring `tests/`, the pushed `metadata.version` must be strictly greater
than `main`'s.
`scripts/check-skill-version-bump.sh` now runs as a pre-push hook on every push, whatever the
target branch. It takes its baseline from the merge-base of the pushed commit with `origin/main`
(local `main` if `origin/main` does not resolve). For each skill directory under
`plugins/*/.apm/skills/` that differs between the pushed commit and that merge-base, ignoring
`tests/`, the pushed `metadata.version` must be strictly greater than the version the skill had at
the merge-base — not the version on `main`'s current tip.
- **Baseline is the merge-base with `main`, not the previous commit.** Readers only ever see
`main` — installs resolve against the default branch (ADR-0018) — so one bump per branch is what
the field owes them. A per-commit check would bump a skill once per commit and inflate the
number past meaning.
- **"Greater", not "exactly one patch higher".** A second `skill-author` pass on the same branch
that bumps again still passes.
- **`tests/` is excluded.** No agent loads it; a fixture-only change does not change the skill.
- **Skills absent from either side are exempt.** New, renamed and merged skills start fresh under
the rules above; deleted skills have nothing to check.
- **Every plugin is covered, `bin` included,** and the gate is repo-local — it is not exported
The baseline is the merge-base, not the previous commit. Readers only ever see `main` — installs
resolve against the default branch (ADR-0018) — so one bump per branch is what the field owes
them. A per-commit check would bump a skill once per commit and inflate the number past meaning.
The rule is "greater", not "exactly one patch higher", so a second `skill-author` pass on the same
branch that bumps again still passes. `tests/` is excluded because no agent loads it; a
fixture-only change does not change the skill. Skills absent from either side are exempt: new,
renamed and merged skills start fresh under the rules above, and deleted skills have nothing to
check. Every plugin is covered, `bin` included, and the gate is repo-local — it is not exported
through `.pre-commit-hooks.yaml`.
**Considered options.** *Declare the field advisory* — cheapest, but concedes the field cannot do
its job. *Drop the field* — rejected by this ADR already, and costlier now. *Check at commit
time against `HEAD`* — rejected for the inflation above.
The gate fails closed rather than passing when it has no trustworthy baseline: when neither
`origin/main` nor `main` resolves, when the pushed commit shares no merge-base with it, and when
only local `main` resolves and the pushed commit is that merge-base, since a local `main` the
pushed commit already contains is no independent record of what shipped. It reads versions with
`python3` and PyYAML and fails with a clear message if either is missing.
**Consequences.** The 17 unbumped skills took a patch bump in the commit that added the gate.
Like `check-release-needed`, the gate only fires on a local `git push` through pre-commit: a merge
made with Gitea's merge button runs no local hooks and is not checked. A typo fix in a skill now
costs a version bump; that is the rule working, not noise.
Three alternatives were rejected. Declaring the field advisory is the cheapest, but concedes the
field cannot do its job. Dropping the field was rejected by this ADR already, and costs more now.
Checking at commit time against `HEAD` was rejected for the inflation described above.
The 17 unbumped skills took a patch bump in the commit that added the gate. A typo fix in a skill
now costs a version bump; that is the rule working, not noise.
The gate differs from `check-release-needed` in when it runs: that hook acts only when pre-commit
reports a push to `main`, so a manual `pre-commit run --hook-stage pre-push` skips it, while this
gate runs there too and checks `HEAD`. The two hooks share both known gaps. A merge made with
Gitea's merge button runs no local hooks, so it is not checked. And pre-commit's pre-push
integration checks only the first ref with new commits in a multi-ref push (`git push origin a b`,
`git push --all`): `_pre_push_ns` in pre-commit's `hook_impl.py` returns after that ref, so the
other refs are pushed unchecked.

View File

@@ -36,6 +36,14 @@ blocked on cutting a premature tag — but it means `--hook-stage pre-push --all
rehearsal of 8 hooks and a skip of the ninth. The script's own header records the same gap for
a PR merged through Gitea's merge button, where no local push happens at all.
A real push has a gap of its own. When one `git push` carries several refs
(`git push origin a b`, `git push --all`), pre-commit runs the pre-push stage once, for the first
ref that has new commits: `_pre_push_ns` in pre-commit's `hook_impl.py` returns as soon as it finds
that ref. The two hooks that read the pushed ref — `check-release-needed` and
`check-skill-version-bump` — therefore check only that ref, and the others are pushed unchecked.
For `check-release-needed`, a multi-ref push whose first ref is not `main` never gates `main` at
all. Push one ref at a time when the gate matters.
## The pre-push gate
Nine hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in.
@@ -75,12 +83,17 @@ drift in generated text.
|---|---|
| `validate-marketplace` | `claude plugin validate --strict` on the root marketplace manifest |
**Skill versioning** (every push, any branch)
| Hook | Guards |
|---|---|
| `check-skill-version-bump` | fails if a skill directory changed since the pushed commit's merge-base with `main` without its `metadata.version` rising above the merge-base's (see [below](#check-skill-version-bump)) |
**Release**
| Hook | Guards |
|---|---|
| `check-release-needed` | on a real `git push` to `main` only — fails if files exposed via `.pre-commit-hooks.yaml` changed since the last tag. A no-op everywhere else, including under `pre-commit run --hook-stage pre-push` (see [the caveat above](#running-the-gates)) |
| `check-skill-version-bump` | on every push — fails if a skill directory changed since the merge-base with `main` without its `metadata.version` rising (see [below](#check-skill-version-bump)) |
Two of these shell out to `apm`: `apm-audit-ci` and `apm-pack-check-clean`. The second is a bare
`apm …` entry and the first is a `bash -c` loop calling `apm` once per package, so without the CLI
@@ -89,20 +102,38 @@ the push dies with an unhelpful "command not found". Install with `apm-install`,
### `check-skill-version-bump`
ADR-0022 makes `metadata.version` mandatory and a skill change carries a bump; `skill-size-check`
only checks the field's presence and shape, so this hook holds the bump itself.
ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bump;
`skill-size-check` only checks the field's presence and shape, so this hook holds the bump itself.
- **Baseline is `git merge-base origin/main <pushed ref>`** (local `main` if `origin/main` does not
resolve). Readers install from `main`, so "changed" means changed against what `main` ships. The
remote branch tip is not the baseline: a second push would excuse an unbumped change the first
push already carried. With no `main` ref or no merge-base, the hook fails closed.
- **It runs on every push and under a manual `pre-commit run --hook-stage pre-push`.** It does not
read `PRE_COMMIT_REMOTE_BRANCH`, so unlike `check-release-needed` the manual rehearsal really
checks it. The pushed commit is `PRE_COMMIT_TO_REF`, or `HEAD` when that is unset.
- **Baseline is the merge-base of the pushed commit with `origin/main`** (local `main` if
`origin/main` does not resolve). The pushed version is compared with the skill's version *at
that merge-base*, not with `main`'s current tip. Readers install from `main`, so "changed" means
changed against the `main` the branch started from. The remote branch tip is not the baseline:
a second push would excuse an unbumped change the first push already carried.
- **It fails closed when it has no trustworthy baseline:** neither `origin/main` nor `main`
resolves; there is no merge-base (shallow clone, unrelated history); or only local `main`
resolves and the pushed commit *is* the merge-base, so local `main` already contains the pushed
commit and says nothing independent about what shipped.
- **A changed skill must end strictly above its baseline version**, compared numerically
(`1.0.10` > `1.0.9`). Any bump size passes. A missing or non-`MAJOR.MINOR.PATCH` version at the
pushed ref fails. Skills absent at the baseline (new, renamed, merged) or at the pushed ref
(deleted) are exempt.
pushed ref fails, and so does a skill directory left without its `SKILL.md`. Versions are
ASCII-only with at most nine digits per part — stricter than `skill-size-check`'s shape check,
so a Unicode digit or an overflowing part cannot pass as a bump. Skills absent at the baseline
(new, renamed, merged) or at the pushed ref (deleted) are exempt.
- **It also fails closed on read errors:** a pushed ref that does not resolve to a commit, or a
`SKILL.md` that `git show` or `python3` cannot read, stops the push with a read-failure message
rather than being reported as a missing version. Frontmatter that reads but does not parse
counts as an invalid version.
- **`<skill>/tests/` is excluded**: no agent loads it, so a test-only change ships nothing.
- **Known gap:** a PR merged through Gitea's merge button runs no local hook, the same gap
`check-release-needed` has.
- **It needs `python3` and PyYAML** to read the frontmatter, and fails with a clear message if
either is missing, for the reasons in
[`python3` and PyYAML are hard requirements](#python3-and-pyyaml-are-hard-requirements).
- **Known gaps, both shared with `check-release-needed`:** a PR merged through Gitea's merge button
runs no local hook; and a multi-ref push checks only its first ref with new commits (see
[Running the gates](#running-the-gates)).
## Skill and agent context gates (ADR-0020)
@@ -416,6 +447,8 @@ which is the exact vacuous-green failure the `python3` check exists to avoid. `p
**Neither requirement generalises to every hook in this repo.** `check-rtk-prefix` needs `python3`
but **not** PyYAML: it reads the markdown body and never touches frontmatter, so it has no scalar to
fold.
`check-skill-version-bump` needs both, for the same reason as `skill-size-check`: it parses
`metadata.version` out of frontmatter.
## Agent files take the description gates, not the body gate