refactor!: carry out the simplification audit across gates, tests, plugins and docs #135
@@ -97,48 +97,67 @@ repos:
|
||||
|
||||
- id: apm-audit-ci
|
||||
name: apm audit --ci
|
||||
description: Run apm's producer-side CI gate over the root manifest AND each of the six plugin packages. Verifies exactly two things per manifest -- apm.yml parses as a valid APM manifest (manifest-parse), and, if it declares dependencies, apm.lock.yaml exists and is consistent (lockfile-exists). It does NOT enforce an org policy and does NOT scan for hidden Unicode; see the comment below for why. Reference:plugins/kyberforge/.apm/skills/apm-workflow/references/audit.md
|
||||
description: Run apm's producer-side CI gate over the root manifest AND each of the six plugin packages. On the root manifest it runs ten checks -- lockfile-exists, ref-consistency, deployment-ledger-owners, deployed-files-present, no-orphaned-packages, skill-subset-consistency, config-consistency, content-integrity, includes-consent, drift -- so it is both a hidden-Unicode scan and a drift gate that replays the install and diffs it. In a plugin package it runs one, lockfile-exists. It does NOT enforce an org policy; see the comment below for why. Reference:plugins/kyberforge/.apm/skills/apm-workflow/references/audit.md
|
||||
entry: bash -c 'for d in . plugins/*/; do (cd "$d" && apm audit --ci) || { echo "apm audit --ci failed in $d" >&2; exit 1; }; done'
|
||||
language: system
|
||||
stages: [pre-push]
|
||||
pass_filenames: false
|
||||
always_run: true
|
||||
# The description above deliberately claims less than this hook's old one
|
||||
# did ("lockfile/policy/hidden-content integrity"), because two of those
|
||||
# three were never happening:
|
||||
# What this hook actually runs, read off apm 0.28.0's own compliance
|
||||
# table by invoking `apm audit --ci` at the repo root and in
|
||||
# plugins/lint/. Long form in docs/spec/gates.md, "apm-audit-ci".
|
||||
#
|
||||
# * POLICY. `apm audit --ci` discovers an org policy from the git remote,
|
||||
# and apm's discovery only understands github.com and Azure DevOps.
|
||||
# This repo's remote is a self-hosted Gitea, so discovery resolves
|
||||
# nothing and the run prints `No org policy found at unknown;
|
||||
# enforcement skipped`. apm's own message suggests
|
||||
# `policy.fetch_failure_default=block` in apm.yml "to fail closed" --
|
||||
# that was tried on a scratch copy and REJECTED: it does not make the
|
||||
# check meaningful, it makes it permanently red. `apm audit --ci` then
|
||||
# exits 1 with `No org policy found at unknown
|
||||
# (policy.fetch_failure_default=block)` on every push, because there is
|
||||
# no org policy to find and no supported way for this remote to serve
|
||||
# one. A gate that can never go green is not a gate. Revisit if this
|
||||
# repo ever gains a policy source apm can actually reach.
|
||||
# * HIDDEN CONTENT. The hidden-Unicode scan is plain `apm audit`, not
|
||||
# `apm audit --ci` (the two are different modes, and --ci refuses to
|
||||
# combine with --file/--strip/--dry-run/PACKAGE). Plain `apm audit`
|
||||
# here reports `No apm.lock.yaml found -- nothing to scan` and exits 0,
|
||||
# so adding it would buy a second vacuous check, not coverage.
|
||||
# * ROOT MANIFEST -- ten checks: lockfile-exists, ref-consistency,
|
||||
# deployment-ledger-owners, deployed-files-present,
|
||||
# no-orphaned-packages, skill-subset-consistency, config-consistency,
|
||||
# content-integrity, includes-consent, drift. It is a drift gate: it
|
||||
# replays the install cache-only and diffs the scratch result against
|
||||
# the working tree. Root lockfile-exists is not vacuous -- the root
|
||||
# declares dependencies, so it reports `Lockfile present`.
|
||||
# * PLUGIN MANIFESTS -- one check: lockfile-exists. Conditional, and
|
||||
# vacuous while every plugin apm.yml declares
|
||||
# `dependencies: {apm: [], mcp: []}`: it reports `No dependencies
|
||||
# declared -- lockfile not required` and arms itself the moment one
|
||||
# does not (verified by adding a git dependency to
|
||||
# plugins/lint/apm.yml). Everything else above is root-only, because
|
||||
# only the root install has a lockfile, a deployment ledger and
|
||||
# deployed files to check. Running the six plugin packages is what
|
||||
# makes lockfile-exists reachable for them at all -- the root-only
|
||||
# invocation audits the root manifest and nothing else.
|
||||
# * HIDDEN CONTENT IS COVERED. content-integrity is that scan; it
|
||||
# reports `No critical hidden Unicode or hash drift detected`. An
|
||||
# earlier revision of this comment said the hook does NOT scan for
|
||||
# hidden Unicode and that adding the scan would buy a second vacuous
|
||||
# check. Both claims were wrong. What is true is that the STANDALONE
|
||||
# mode differs: plain `apm audit` (--ci refuses to combine with
|
||||
# --file/--strip/--dry-run/PACKAGE) run in a plugin directory reports
|
||||
# `No apm.lock.yaml found -- nothing to scan` and exits 0, because
|
||||
# only the root has a lockfile.
|
||||
# * MANIFEST-PARSE IS NOT A CHECK in apm 0.28.0's table, and an earlier
|
||||
# revision of this comment named it as one. Parsing is still
|
||||
# enforced -- a dependency entry missing its git/path/registry field
|
||||
# fails with `Cannot parse apm.yml` -- but it fails the invocation
|
||||
# before the table is built, so it never appears as a row.
|
||||
# * POLICY IS NOT ENFORCED. `apm audit --ci` discovers an org policy
|
||||
# from the git remote, and apm's discovery only understands
|
||||
# github.com and Azure DevOps. This repo's remote is a self-hosted
|
||||
# Gitea, so discovery resolves nothing and the run prints `No org
|
||||
# policy found at unknown; enforcement skipped`. apm's own message
|
||||
# suggests `policy.fetch_failure_default=block` in apm.yml "to fail
|
||||
# closed" -- that was tried on a scratch copy and REJECTED: it does
|
||||
# not make the check meaningful, it makes it permanently red. `apm
|
||||
# audit --ci` then exits 1 with `No org policy found at unknown
|
||||
# (policy.fetch_failure_default=block)` on every push, because there
|
||||
# is no org policy to find and no supported way for this remote to
|
||||
# serve one. A gate that can never go green is not a gate. Revisit if
|
||||
# this repo ever gains a policy source apm can actually reach.
|
||||
#
|
||||
# What IS left is worth keeping, and is now run against seven manifests
|
||||
# instead of one. lockfile-exists is conditional -- it is vacuous while
|
||||
# every apm.yml declares `dependencies: {apm: [], mcp: []}`, and it arms
|
||||
# itself the moment one does not (verified: adding a git dependency to
|
||||
# plugins/lint/apm.yml fails with `apm.yml declares dependencies but
|
||||
# apm.lock.yaml is absent`). manifest-parse is unconditional and fires on
|
||||
# any malformed manifest (verified: a dependency entry missing its
|
||||
# git/path/registry field fails with `Cannot parse apm.yml`). Running the
|
||||
# six plugin packages is what makes either reachable for them at all --
|
||||
# the root-only invocation audits the marketplace manifest and nothing
|
||||
# else. Costs ~0.5s per package, needs no network (checked under
|
||||
# `unshare -rn`) -- consistent with every other pre-push hook: none of
|
||||
# them need the network (see README.md's "Offline?" section).
|
||||
# Costs ~0.5s per package. Needs no network ONCE `apm install` has
|
||||
# populated apm_modules/ -- the root marketplace has no remote package
|
||||
# entries, so the install replay is cache-only. On a FRESH CLONE there
|
||||
# is no cache: deployed-files-present fails outright, and drift and
|
||||
# config-consistency clone from the holocron remote. See README.md's
|
||||
# "Offline?" section.
|
||||
|
||||
- id: check-apm-agents-valid
|
||||
name: Validate real APM agent files
|
||||
@@ -167,9 +186,12 @@ repos:
|
||||
pass_filenames: false
|
||||
always_run: true
|
||||
|
||||
# check-vale-style-sync was removed by ADR-0025. Only 6 of its 17
|
||||
# assertions diffed skill-audit's Vale copy against agent-audit's; the
|
||||
# merge into factory-audit leaves one copy, so those are moot. The other
|
||||
# check-vale-style-sync was removed by ADR-0025. Of its 17 assertion
|
||||
# sites only 2 actually diffed skill-audit's Vale copy against
|
||||
# agent-audit's, and 4 more existed solely so the script could locate the
|
||||
# two copies -- a real REPO_ROOT, non-stale .apm/ paths, both copies
|
||||
# present (ADR-0025:285-287). The merge into factory-audit leaves one
|
||||
# copy, so all 6 are moot. The other
|
||||
# 11 moved into tests/test-vale-wrap.sh (case 0, cases 28-31, its
|
||||
# Vale-absent skip, and case 32 for the one-plugin narrowing guard),
|
||||
# which run-tests runs here at
|
||||
|
||||
@@ -27,7 +27,7 @@ Skills are **not** deployed by `install.sh`. They are distributed as plugins and
|
||||
|
||||
Skills, agents, MCP servers, and hooks are distributed as self-contained plugin units under `plugins/`, installed independently via `apm install`, here and in any consuming repo (ADR-0018). Each unit is an **apm package**: `plugins/<name>/apm.yml` plus a hand-authored `plugins/<name>/.apm/{skills,agents,hooks,commands,instructions,extensions}/` tree (ADR-0015). There is no per-plugin `plugin.json` at all — apm reads `apm.yml`, and the repo's one generated manifest, `.claude-plugin/marketplace.json`, is compiled from that source.
|
||||
|
||||
Self-contained is a hard constraint, not a description: a file reference inside `.apm/skills/<name>/` may not reach outside that skill's own directory, and there is no cross-skill sharing mechanism to reach for instead. That is why the Vale styles ship inside the one skill that uses them, `factory-audit/assets/vale/` (ADR-0014, ADR-0025), and why ADR-0020's constants are copied into two validators — the plugin's `validate.sh` and the repo's `scripts/skill-size-check.sh` — rather than sourced from one. The constraint used to be explained by Claude Code's plugin cache-install copying a plugin to a cache; that is no longer the reason and never was the only one. It is stated independently for APM package mode by the agentskills.io spec (`plugins/kyberforge/.apm/skills/skill-author/references/deployment-modes.md`), which is why ADR-0024 consequence 6 pins it as a negative result: ending native install did not relax it, and it is not to be re-litigated on the assumption that it did.
|
||||
Self-contained is a hard constraint, not a description: a file reference inside `.apm/skills/<name>/` may not reach outside that skill's own directory, and there is no cross-skill sharing mechanism to reach for instead. That is why the Vale styles ship inside the one skill that uses them, `factory-audit/assets/vale/` (ADR-0014, ADR-0025), and why ADR-0020's constants are copied rather than sourced from one place: `scripts/skill-size-check.sh` carries them, and so do `factory-audit`'s mode libraries — `scripts/lib-checks-skill.sh:313-316` all four, `scripts/lib-checks-agent.sh:164-165` the two description ones. The plugin's `validate.sh` carries none of them; it sources the library its mode selects. The constraint used to be explained by Claude Code's plugin cache-install copying a plugin to a cache; that is no longer the reason and never was the only one. It is stated independently for APM package mode by the agentskills.io spec (`plugins/kyberforge/.apm/skills/skill-author/references/deployment-modes.md`), which is why ADR-0024 consequence 6 pins it as a negative result: ending native install did not relax it, and it is not to be re-litigated on the assumption that it did.
|
||||
|
||||
Which apm package a new skill belongs in follows from what each one is scoped to. The boundary that matters most in practice is `core` vs `kyberforge`: `core` is the home for cross-cutting, repo-agnostic utility skills that a consumer would want against *their* repo, while `kyberforge` is meta-tooling for the holocron marketplace itself. A skill that authors a target repo's `AGENTS.md` is `core`; a skill that audits a `SKILL.md` against this marketplace's contract is `kyberforge`.
|
||||
|
||||
|
||||
@@ -98,19 +98,32 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu
|
||||
|
||||
- **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 the manual rehearsal really checks it. The pushed commit is `PRE_COMMIT_TO_REF`, or `HEAD` when that is unset.
|
||||
- **"Changed" is measured from the merge-base of the pushed commit with `origin/main`** (local
|
||||
`main` if `origin/main` does not resolve). 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.
|
||||
- **A changed skill's version must beat two baselines**: its version at that merge-base *and* its
|
||||
- **"Changed" is measured from the merge-bases of the pushed commit with `origin/main`** (local
|
||||
`main` if `origin/main` does not resolve), resolved with **`git merge-base --all`** — all of
|
||||
them, not the single one git would otherwise pick. 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.
|
||||
- **With more than one base, the changed-skill sets are intersected.** A criss-cross history —
|
||||
`main` merges a branch while that branch merges a `main` commit — has two merge-bases, and which
|
||||
one a bare `git merge-base` prints is an implementation detail, so picking one made the verdict a
|
||||
coin flip: a skill already identical to `main` was reported `(not above merge-base)` whenever the
|
||||
losing base was chosen. A skill therefore counts as changed only when it differs from **every**
|
||||
base; differing from none of them, or from only some, means a base already carries the pushed
|
||||
content. A skill that does count as changed must then beat the version at every base it exists
|
||||
at. Both directions are conservative: the intersection cannot exempt a skill that changed since
|
||||
all of `main`'s reachable history, and requiring every base keeps the ratchet.
|
||||
- **A changed skill's version must beat two baselines**: its version at each merge-base *and* its
|
||||
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, 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
|
||||
tip is held to the merge-bases alone, and so is one whose directory at the pushed commit is the
|
||||
**same tree object** as at the tip — compared as object ids, because a tree id *is* the content
|
||||
whatever route the history took to it. That skill ships exactly what `main` ships, so there is
|
||||
nothing for a bump to announce. The intersection does not already cover it: it exempts only when
|
||||
some base carries the content, which a criss-cross history gives and a cherry-pick of a fix
|
||||
`main` already has does not. When `main` has not moved, the tip is itself a base and the skill is
|
||||
checked once. Each failure line names the baseline it missed: `(not above merge-base)`,
|
||||
`(not above merge-base <sha>)` when there is more than one base to tell apart, 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`
|
||||
resolves; there is no merge-base (shallow clone, unrelated history); or only local `main`
|
||||
@@ -428,7 +441,8 @@ if the library is missing or defines no resolver.
|
||||
|
||||
`tests/test-adr0020-contract.sh` pins that arrangement: the library carries the only marker pair,
|
||||
the hook carries none, the hook fails closed without the library, and a sentinel planted in a copied
|
||||
library proves the hook executes the library's text. One of its assertions was green on a
|
||||
library proves the hook executes the library's text, and a later block pins every repo-authored
|
||||
pre-commit hook's `entry` and `stages`. One of its assertions was green on a
|
||||
defect it named. "`validate.sh` sources the resolver in **both mode branches**" was implemented as a
|
||||
file-wide `grep -Ec … -ge 2`, which cannot see a branch at all: delete the `agent)` arm's source line
|
||||
and duplicate the `skill)` arm's, and the file-wide count is still 2 and the assertion still passes,
|
||||
@@ -436,11 +450,14 @@ with the agent path running no resolver or some other one. It is now a **per-arm
|
||||
each arm of `validate.sh`'s `case "$MODE" in` block must carry exactly one `source` line inside its
|
||||
own body, and the file must carry exactly those two — with a mutation self-test that performs that
|
||||
exact count-preserving edit on a copy and requires the check to fail on it. The suite's case count
|
||||
runs **28 → 27 → 29**, and is **29** at HEAD: 28 at `620f20b` (the ADR-0025 merge), 27 after
|
||||
`4de5b6b` retired the `.pre-commit-hooks.yaml` export, and 29 after `ef27c97` replaced the two-copy
|
||||
hash and its line-count floor with the six one-copy assertions above. There are two 2026-09-16
|
||||
runs **28 → 27 → 29 → 44**, and is **44** at HEAD: 28 at `620f20b` (the ADR-0025 merge), 27 after
|
||||
`4de5b6b` retired the `.pre-commit-hooks.yaml` export, 29 after `ef27c97` replaced the two-copy
|
||||
hash and its line-count floor with the six one-copy assertions above, and 44 after `384756b` added
|
||||
the hook-wiring block. An earlier revision of this section stopped the chain at 29 and called that
|
||||
the figure at HEAD; it was written before `384756b`. There are two 2026-09-16
|
||||
changes here, not one, which is what an earlier revision of this section conflated. Each figure is
|
||||
`bash tests/test-adr0020-contract.sh` run in a worktree at that commit, reading its `Results:` line.
|
||||
`bash tests/test-adr0020-contract.sh` run at that commit — in a worktree for the historical ones —
|
||||
reading its `Results:` line.
|
||||
An earlier revision also opened the chain at 25; that predates the branch squash, no reachable
|
||||
commit reproduces it, and it is dropped as unverifiable rather than carried.
|
||||
|
||||
@@ -482,8 +499,10 @@ against synthetic `mktemp` fixtures — it had never run against the agent files
|
||||
how ADR-0016 could be amended to bless a `disallowedTools` frontmatter field while `validate.sh`'s
|
||||
allowlist still rejected it: spec and enforcer disagreed and every gate stayed green.
|
||||
|
||||
Agents take the ADR-0020 **description** gates (`factory-audit`'s `validate.sh` holds its own copy of
|
||||
those two constants) and, deliberately, **no body word gate**. A skill body is loaded into the
|
||||
Agents take the ADR-0020 **description** gates and, deliberately, **no body word gate**. The two
|
||||
description constants `factory-audit` applies to an agent live in `scripts/lib-checks-agent.sh:164-165`;
|
||||
an earlier revision of this line put them in its `validate.sh`, which carries none of them (see
|
||||
[Duplicated constants](#duplicated-constants)). A skill body is loaded into the
|
||||
caller's context and competes with the live conversation; an agent body becomes the system prompt of
|
||||
a *fresh* context. The rationale for the 900-word FAIL does not transfer. A bats test pins that
|
||||
absence for the agent path of `factory-audit`'s validator — adding a body gate there contradicts the
|
||||
@@ -910,11 +929,12 @@ An explicit `--config` from any other caller still wins, in all three argv forms
|
||||
`--config=/abs`, `--config=rel`), and a relative one resolves against the caller's cwd — matching
|
||||
bare `vale`, not the repo root.
|
||||
|
||||
Both audit skills' Step 1 passes no `--config` either. Step 1 resolves the script relative to the
|
||||
`factory-audit`'s Step 1 passes no `--config` either. Step 1 resolves the script relative to the
|
||||
skill's own directory so the call works from an installed plugin cache; a relative `--config`
|
||||
alongside it would resolve against the cwd instead, yielding `E100 Runtime error … does not exist`
|
||||
and exit 2 — which both skills' fallback misreads as "vale unavailable" and silently downgrades to
|
||||
full LLM judgment.
|
||||
and exit 2 — which the skill's fallback misreads as "vale unavailable" and silently downgrades to
|
||||
full LLM judgment. An earlier revision wrote this paragraph in the plural, for the `skill-audit` /
|
||||
`agent-audit` pair ADR-0025 merged; there is one Step 1 now.
|
||||
|
||||
`tests/test-vale-wrap.sh` regression-tests this against `factory-audit`'s copy — the only one left.
|
||||
Its fixtures are all `SKILL.md`-shaped, and that copy's `.vale.ini` carries the matching glob section
|
||||
|
||||
Reference in New Issue
Block a user