diff --git a/AGENTS.md b/AGENTS.md index 8a520a3..3aeb20d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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//.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. diff --git a/README.md b/README.md index 348aa9e..cf207d8 100644 --- a/README.md +++ b/README.md @@ -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: 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 34e44d1..f9352d3 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 @@ -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 diff --git a/docs/adr/0022-skill-metadata-version-is-mandatory.md b/docs/adr/0022-skill-metadata-version-is-mandatory.md index 9ba050a..b507ca4 100644 --- a/docs/adr/0022-skill-metadata-version-is-mandatory.md +++ b/docs/adr/0022-skill-metadata-version-is-mandatory.md @@ -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 - through `.pre-commit-hooks.yaml`. +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. diff --git a/docs/spec/gates.md b/docs/spec/gates.md index 3fbc1a0..ff5fbdb 100644 --- a/docs/spec/gates.md +++ b/docs/spec/gates.md @@ -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 `** (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. - **`/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