diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 395d3c5..7985edf 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -237,9 +237,11 @@ repos: entry: scripts/check-rtk-prefix.sh language: script files: '^plugins/[^/]+/\.apm/(skills/.*\.md|agents/.*\.agent\.md)$' - # README.md is excluded on purpose, not by oversight. A skill-directory - # README is consumer-facing prose that no agent ever loads, and the - # `git clone` lines in the six tests/README.md files are setup + # README.md is excluded on purpose, not by oversight. The 12 README.md + # files still in scope sit in a skill's scripts/, tests/ and assets/ + # subdirectories -- consumer-facing prose that no agent ever loads (the + # skill-directory READMEs this was first written for are deleted) -- and + # the `git clone` lines in the six tests/README.md files are setup # instructions for a third party who has no rtk installed. Prefixing # those would be actively wrong -- see ADR-0023's consumer section. exclude: '(^|/)README\.md$' diff --git a/README.md b/README.md index c954b13..234acf0 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Content ships as six installable plugins, each an apm (Agent Package Manager) pa | `providers/claude-code/` | Claude Code adapter, deployed to `~/.claude/` via `scripts/install.sh` | | `core/` | Provider-agnostic always-on content — `core/AGENTS.md` and `core/instructions/` | | `docs/` | Specs (`docs/spec/`), architectural decisions (`docs/adr/`), governance, research, and notes | -| `scripts/` | Install, sync, and check scripts used by the git hooks | +| `scripts/` | Install and check scripts used by the git hooks | | `tests/` | `run-tests.sh`, `run-bats.sh`, the `test-*.sh` suites, and the bats submodules | The six plugins: @@ -86,7 +86,7 @@ pre-commit run --hook-stage pre-push --all-files See [`docs/spec/gates.md`](docs/spec/gates.md) for what each hook enforces and why. -**Offline?** No pre-push hook needs the network: root `apm.yml`'s marketplace has no remote package entries (the last one, `mattpocock-skills`, was removed), so `apm-pack-check-clean` resolves everything from local sources. All pre-push hooks pass offline. +**Offline?** No pre-push hook needs the network **once `apm install` has populated `apm_modules/`**. Root `apm.yml`'s marketplace has no remote package entries (the last one, `mattpocock-skills`, was removed), so `apm-pack-check-clean` resolves everything from local sources, and `apm-audit-ci`'s install-replay is cache-only against a populated install. On a **fresh clone** there is no cache: `apm-audit-ci`'s `deployed-files-present` fails outright, and its `drift` and `config-consistency` checks clone from the holocron remote. The offline guarantee is a property of a populated `apm_modules/`, not of the hook set — run `apm install` once on a new checkout and it holds from then on. ## Editing plugin content diff --git a/docs/spec/gates.md b/docs/spec/gates.md index 55f79da..1d06e1e 100644 --- a/docs/spec/gates.md +++ b/docs/spec/gates.md @@ -57,8 +57,9 @@ Eight hooks, grouped below by what they guard rather than by the order `.pre-com | `check-scope-walkup-sync` | `validate.sh`, `validate-provenance.sh`, `new-agent.sh` and `new-skill.sh`'s four independent `$HOME`/`.git`/`apm.yml` walk-up ports still agree behaviorally | | `check-executables-allow-sync` | root `apm.yml`'s `executables.allow` key names kyberforge's actual version (see [apm gates](#apm-gates)) | -`check-executables-allow-sync` is the odd one in this group: it guards a *silent failure* rather than -drift in generated text. +`check-executables-allow-sync` is the odd one in this group: the drift it guards is in a +hand-written key rather than in generated text, and it is a record-keeping gate — the grant itself is +version-blind, so a stale key deploys fine (see [apm gates](#apm-gates)). **Artifact validators** @@ -105,7 +106,10 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu version at the tip of the same `main` ref (ADR-0022's second 2026-09-16 amendment). The tip check stops two branches that make the same bump (`1.0.0` → `1.0.1`) with different content from both landing, since the identical version lines merge without a conflict. A skill absent at the - tip is held to the merge-base alone; when `main` has not moved, the two are the same commit. Each + tip is held to the merge-base alone, and so is one whose directory at the pushed commit is the + *same tree object* as at the tip: it ships exactly what main ships, whatever route the history + took there — a criss-cross merge, a cherry-pick, a backport — so there is nothing for a bump to + announce. When `main` has not moved, the two baselines are the same commit. Each failure line names the baseline it missed: `(not above merge-base)` or `(not above origin/main tip)`. The tip is `origin/main` as last fetched. - **It fails closed when it has no trustworthy baseline:** neither `origin/main` nor `main` @@ -634,8 +638,10 @@ boundary, and a stricter form would only move the same trust to a different stri - **Prose bullets.** Most of `branch-operations.md`, `merging.md` and `rewrite-history.md` instruct in list items, not fences. Those are clause-1 sites the gate cannot see, because it cannot distinguish them from clause-2 mentions in the same list. -- **`README.md`, excluded by pattern.** A skill-directory README is consumer-facing prose no agent - loads, and the `git clone https://github.com/bats-core/…` lines in the six `tests/README.md` +- **`README.md`, excluded by pattern.** The skill-directory READMEs the exclusion was first written + for are deleted; what it still covers is the 12 `README.md` files inside a skill's `scripts/`, + `tests/` and `assets/` subdirectories — consumer-facing prose no agent loads — and the + `git clone https://github.com/bats-core/…` lines in the six `tests/README.md` files are setup instructions for a third party who has no `rtk`. Prefixing those would be actively wrong, not merely noisy — see ADR-0023's consumer section. - **Quoting.** The line splitter breaks on `;`, `|`, `&&`, `||`, `$(` and backticks without tracking @@ -939,8 +945,8 @@ pass, and a skip fails the push. `test-vale-wrap.sh` without Vale skips only its Vale-dependent cases, not the whole suite. The cases that are plain greps and awk over the Vale config and `.pre-commit-config.yaml` still run: case 0, 16, 26, -27, the static half of 28, 31 Parts A and B, 32 and 34. A static failure exits 1, because a -real defect is not a setup error. Only an all-static-pass run exits 77. +27, the static half of 28, 31 Parts A and B, 32, 34 and the static half of 35. A static failure +exits 1, because a real defect is not a setup error. Only an all-static-pass run exits 77. ### Mentioning banned phrasing without tripping the rule @@ -1010,9 +1016,10 @@ near-miss negatives it must leave alone, and the suite from 5 cases to **7**. an `echo` or `printf` feeding any of them is the same race. Those are guarded by **convention** — absorb the writer's status with `|| true`, or take the verdict from a here-string — and deliberately not by this test: most legitimate uses of them in this tree are already absorbed, and the scanner -cannot see absorption from the pipeline text alone, so flagging them would be noise. Two live -`grep … | head -1` sites (`tests/test-vale-wrap.sh:620` and `:1046`) were fixed by hand with that -idiom. Pipes from a non-builtin writer (`run_wrap … | grep -q`) are out of scope for the same reason: +cannot see absorption from the pipeline text alone, so flagging them would be noise. The three live +`grep … | head -1` sites in `tests/test-vale-wrap.sh` — in `unguarded_expansions()`, in case 20B's +`--output line` line-number read, and in case 28's per-file `RESULTS28` lookup — carry that idiom by +hand. Pipes from a non-builtin writer (`run_wrap … | grep -q`) are out of scope for the same reason: in practice they either absorb the writer's exit status with `|| true` or write only once, at exit. **Known limitation: heredoc bodies are scanned as code.** A `cat <<'EOF'` body containing a diff --git a/scripts/check-executables-allow-sync.sh b/scripts/check-executables-allow-sync.sh index 51f46c0..86457ca 100755 --- a/scripts/check-executables-allow-sync.sh +++ b/scripts/check-executables-allow-sync.sh @@ -2,11 +2,15 @@ set -euo pipefail # Fails the push when root apm.yml's executables.allow key stops naming -# kyberforge's actual version. apm matches that key by exact dict lookup -# (apm_cli/security/executables.py) — a version bump that misses the key -# update deploys nothing, with no error anywhere. See ADR-0019, "The allow -# key is version-pinned, and that is a live failure mode", for the full -# argument; nothing else in the pre-push gate compares these two files. +# kyberforge's actual version. The grant itself is version-blind — apm matches +# the version-less name alongside '#' (exec_gate.py builds +# the candidate list, _map_grants matches it) — so a stale key keeps granting +# kyberforge's hooks/ and bin/ and deployment does not break. What this gate +# buys is repo-level, not apm-level: a version bump without the matching key +# edit fails this repo's own pre-push, so the key stays an accurate record of +# what was approved. See ADR-0019's 2026-09-19 correction and docs/spec/gates.md, +# "check-executables-allow-sync"; nothing else in the pre-push gate compares +# these two files. # # Run from repo root or pass REPO_ROOT as arg. @@ -213,10 +217,10 @@ if [[ -n "$STALE_KEYS" ]]; then echo " Found instead:" >&2 printf '%s\n' "$STALE_KEYS" | sed 's/^/ /' >&2 fi -echo " Why: apm matches this key by exact dict lookup — there is no wildcard and no version-less" >&2 -echo " form — so a key naming any other version silently stops granting kyberforge's hooks/" >&2 -echo " and bin/. The SessionStart hook then stops deploying and the apm install goes stale" >&2 -echo " with no error anywhere (ADR-0019, 'The allow key is version-pinned')." >&2 +echo " Why: nothing is broken right now — apm's grant is version-blind, so a key naming another" >&2 +echo " version still grants kyberforge's hooks/ and bin/. This gate is a repo-level record" >&2 +echo " check: a version bump without the matching key edit fails here, which is what keeps" >&2 +echo " the key an accurate record of what was approved (ADR-0019, 2026-09-19 correction)." >&2 echo " Fix: bump the key in root apm.yml to '$EXPECTED_KEY:' — the version bump in" >&2 echo " plugins/kyberforge/apm.yml is not complete without it." >&2 exit 1 diff --git a/scripts/check-skill-version-bump.sh b/scripts/check-skill-version-bump.sh index 36afbf7..83a224e 100755 --- a/scripts/check-skill-version-bump.sh +++ b/scripts/check-skill-version-bump.sh @@ -36,7 +36,11 @@ set -euo pipefail # without a conflict, so the merge-base alone would let main ship both under # one version. When
has not moved since the merge-base, the two # baselines are one commit and the skill is checked once. The tip is read as -# last fetched. +# last fetched. A skill whose directory at the pushed commit is the SAME TREE +# OBJECT as at the tip skips the tip comparison: it ships exactly what main +# ships, so there is nothing to announce. The merge-base intersection catches +# that only when some base carries the content — true of the criss-cross shape +# above, false of a branch that cherry-picks a fix main already has. # # Pushing main itself: with origin/main as the baseline, a push of main diffs # the new commits against what the remote already has, so it is covered. A @@ -281,6 +285,17 @@ in_tree() { git rev-parse --verify -q "$1:$2" > /dev/null } +# same_subtree : both name with the same +# object, so the two commits ship byte-identical content there. Compared as +# object ids rather than by diffing: a tree id is the content, whatever route +# the history took to it. A path missing on either side is not a match. +same_subtree() { + local a b + a="$(git rev-parse --verify -q "$1:$3")" || return 1 + b="$(git rev-parse --verify -q "$2:$3")" || return 1 + [[ "$a" == "$b" ]] +} + OFFENDERS=() for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do # Bases the skill exists at, in merge-base order, with the version read at @@ -306,7 +321,18 @@ for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do break fi done - if [[ "$tip_is_base" == false ]] && in_tree "$MAIN_TIP" "$dir/SKILL.md"; then + # A skill byte-identical to the tip's copy of it is already what main ships, + # so there is nothing left for a bump to announce. The merge-base rule alone + # would already have exempted it, but only when a base carries that same + # content — which a criss-cross history gives and a linear one does not. A + # branch cut before a fix landed on main and then cherry-picking that fix has + # one merge-base, predating the fix, so the skill counts as changed against + # it and reaches the tip comparison carrying exactly the tip's version. Same + # content, same version, and the only escapes would be a spurious bump — + # leaving main carrying two versions of identical content — or a rebase the + # push does not otherwise need. + if [[ "$tip_is_base" == false ]] && in_tree "$MAIN_TIP" "$dir/SKILL.md" \ + && ! same_subtree "$MAIN_TIP" "$PUSHED_COMMIT" "$dir"; then at_tip=true fi # Absent at every baseline: new, renamed-to, or merged-into. Exempt. @@ -317,15 +343,22 @@ for dir in ${SKILL_DIRS[@]+"${SKILL_DIRS[@]}"}; do tip_ver="" if $at_tip; then version_at "$MAIN_TIP" "$dir/SKILL.md"; tip_ver="$VERSION"; fi + # The baseline the two messages below name. A skill added on main after the + # branch was cut is absent at every merge-base, so no base names a version + # while the tip does — and the tip's is the version the push is actually held + # to. Reporting "none" there sends the author to the natural 1.0.0 and costs + # them a second blocked push on the same mistake. + report_ver="${first_base_ver:-$tip_ver}" + if ! in_tree "$PUSHED_COMMIT" "$dir/SKILL.md"; then - OFFENDERS+=("$dir: SKILL.md missing at $PUSHED_REF (baseline: ${first_base_ver:-none})") + OFFENDERS+=("$dir: SKILL.md missing at $PUSHED_REF (baseline: ${report_ver:-none})") continue fi version_at "$PUSHED_COMMIT" "$dir/SKILL.md" cur_ver="$VERSION" if [[ -z "$cur_ver" ]]; then - OFFENDERS+=("$dir: metadata.version missing or not MAJOR.MINOR.PATCH at $PUSHED_REF (baseline: ${first_base_ver:-none})") + OFFENDERS+=("$dir: metadata.version missing or not MAJOR.MINOR.PATCH at $PUSHED_REF (baseline: ${report_ver:-none})") continue fi # Named by sha only when there is more than one base to tell apart; a diff --git a/scripts/deploy-manifest.sh b/scripts/deploy-manifest.sh index 4d66a7f..8fc971f 100644 --- a/scripts/deploy-manifest.sh +++ b/scripts/deploy-manifest.sh @@ -20,5 +20,3 @@ DEPLOY_EXECUTABLES=( DEPLOY_DIRS=( "core:.claude/core" ) - -# Provider skill adapters are declared in providers/*/provider-manifest.sh, not here. diff --git a/tests/test-skill-version-bump.sh b/tests/test-skill-version-bump.sh index a5a2257..da9977a 100755 --- a/tests/test-skill-version-bump.sh +++ b/tests/test-skill-version-bump.sh @@ -633,6 +633,77 @@ write_skill "$F" demo alpha 'version: "1.0.1"' "later body"; commit "$F" F3 expect_fail "an unbumped edit on a criss-cross branch still fails, naming the baseline sha" \ "alpha: 1\.0\.1 -> 1\.0\.1 \(not above merge-base [0-9a-f]{40}\)" "$F" +echo "" +echo "--- 41. a cherry-picked fix identical to main's tip passes ---" +# Case 40 pins the same property — content identical to main's tip ships +# nothing — for a criss-cross history, where the merge-base intersection alone +# already exempts the skill because one base carries that content. This is the +# LINEAR shape, where no base does: the branch was cut before the fix landed on +# main and then cherry-picked it, so the single merge-base predates the fix and +# the skill reaches the tip comparison carrying exactly the tip's version. The +# only escapes would be a spurious 1.0.2 — leaving main with two versions of +# identical content — or a rebase the push does not otherwise need. +# +# C0 alpha 1.0.0 +# +-- main: FIX bumps alpha to 1.0.1 (origin/main) +# +-- feature: cherry-picks FIX +F="$(make_fixture)" +(cd "$F" && git checkout -q main) +write_skill "$F" demo alpha 'version: "1.0.1"' "fixed body"; commit "$F" "fix alpha" +FIX_SHA="$(cd "$F" && git rev-parse HEAD)" +# -x: without it the picked commit can come out byte-identical to FIX — same +# tree, same parent, same author and committer second — and git reuses the sha, +# so the branch silently fast-forwards onto main and the case under test is +# gone. The trailer -x adds guarantees a distinct commit. +(cd "$F" && git update-ref refs/remotes/origin/main main && git checkout -q feature \ + && git cherry-pick -x "$FIX_SHA" > /dev/null) +if [[ "$(cd "$F" && git rev-parse HEAD)" != "$FIX_SHA" ]]; then + pass "fixture check: the cherry-pick made a distinct commit, not a fast-forward onto main" +else + fail "fixture check: the branch fast-forwarded onto main, so the tip is the merge-base" +fi +if [[ "$(cd "$F" && git merge-base --all origin/main HEAD | wc -l)" -eq 1 ]]; then + pass "fixture check: the linear history has exactly one merge-base" +else + fail "fixture check: expected one merge-base, got $(cd "$F" && git merge-base --all origin/main HEAD)" +fi +if [[ -z "$(cd "$F" && git diff origin/main HEAD -- plugins)" ]]; then + pass "fixture check: nothing under plugins/ differs between origin/main and the branch" +else + fail "fixture check: plugins/ differs, so this is not the case under test" +fi +expect_pass "a cherry-picked skill identical to main's tip passes without a further bump" "$F" +# The ratchet still holds on the same shape: a further edit is no longer +# identical to the tip, so the tip rule applies again. +write_skill "$F" demo alpha 'version: "1.0.1"' "later body"; commit "$F" +expect_fail "an unbumped edit on top of the cherry-pick still fails against the tip" \ + "alpha: 1\.0\.1 -> 1\.0\.1 \(not above origin/main tip\)" "$F" + +echo "" +echo "--- 42. the named baseline is the tip when no merge-base carries the skill ---" +# A skill added on main after the branch was cut is absent at every merge-base, +# so only the tip names a version — and the tip's is the version the push is +# held to. Reporting "none" sends the author to the natural 1.0.0 and costs a +# second blocked push on the same mistake. +F="$(make_fixture)" +(cd "$F" && git checkout -q main) +write_skill "$F" demo delta 'version: "3.2.1"' "main's delta"; commit "$F" "add delta on main" +(cd "$F" && git update-ref refs/remotes/origin/main main && git checkout -q feature) +write_skill "$F" demo delta "" "branch delta"; commit "$F" "add delta on branch" +expect_fail "a missing version names the tip's version, not 'none'" \ + "delta: metadata\.version missing or not MAJOR\.MINOR\.PATCH at HEAD \(baseline: 3\.2\.1\)" "$F" +expect_fail "the baseline is never reported as none while the tip carries one" \ + "delta: metadata\.version missing or not MAJOR\.MINOR\.PATCH at HEAD \(baseline: [0-9]" "$F" +# The same message shape for the SKILL.md-missing branch of the report. +F="$(make_fixture)" +(cd "$F" && git checkout -q main) +write_skill "$F" demo delta 'version: "3.2.1"' "main's delta"; commit "$F" "add delta on main" +(cd "$F" && git update-ref refs/remotes/origin/main main && git checkout -q feature) +mkdir -p "$F/plugins/demo/.apm/skills/delta/references" +echo "ref" > "$F/plugins/demo/.apm/skills/delta/references/x.md"; commit "$F" "delta without SKILL.md" +expect_fail "a missing SKILL.md names the tip's version too" \ + "delta: SKILL\.md missing at HEAD \(baseline: 3\.2\.1\)" "$F" + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]]