Four claims in the spec and the hook config stated as fact what the tools
do not do:
- gates.md:485 said factory-audit's validate.sh "holds its own copy of"
the ADR-0020 constants. gates.md:411-413, twenty lines earlier, said it
carries none of them and named the mode libraries. The libraries are
right: lib-checks-skill.sh:313-316 and lib-checks-agent.sh:164-165.
architecture.md repeated the same error.
- gates.md stated the case count for test-adr0020-contract.sh as "29 at
HEAD", explicitly presented as measured. Running it prints 44; 384756b
added the hook-wiring assertions after the text was written.
- gates.md:913 and :916 described "Both audit skills'" behaviour in the
present tense, three and six lines above :919 saying factory-audit's is
the only copy left.
- The apm-audit-ci block named manifest-parse as a check, said the hook
does not scan for hidden Unicode, and called root lockfile-exists
vacuous. apm 0.28.0 runs ten checks, content-integrity does scan for
hidden Unicode, and there is no manifest-parse row.
Also: the version-bump gate's baseline is documented as the single
merge-base it is not -- it resolves every base with merge-base --all,
intersects the changed-skill sets, exempts a tree-identical skill, and
emits a third sha-suffixed failure form. The gate's own header documents
this correctly; the spec did not. Behaviour is unchanged.
The "none of them need the network" line added on this branch cited a
README section that says the opposite for a fresh clone, and the
check-vale-style-sync rationale said 6 of 17 assertions diffed the Vale
copies where ADR-0025 says 2 diffed and 4 more only located them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NwD8Egs5r4ndqeFLmhusX2
1210 lines
80 KiB
Markdown
1210 lines
80 KiB
Markdown
# Enforcement gates
|
||
|
||
Reference for this repo's pre-commit and pre-push hooks: what each one guards, what its numbers
|
||
mean, and which shapes were tried and rejected. Read it when a gate fails, before changing anything
|
||
in `.pre-commit-config.yaml`, or before "fixing" something that looks like an inconsistency — several
|
||
of the oddities documented here are load-bearing and have already been re-litigated once.
|
||
|
||
`AGENTS.md` carries only the operative rules an agent needs in the moment. The reasoning lives here.
|
||
|
||
---
|
||
|
||
## Running the gates
|
||
|
||
| Command | Scope |
|
||
|---|---|
|
||
| `pre-commit run --all-files` | the commit-stage hooks |
|
||
| `pre-commit run --hook-stage pre-push --all-files` | the push gate, one command |
|
||
| `pre-commit run skill-size-check --all-files` | just the ADR-0020 size/context gates |
|
||
|
||
Install hooks via `pc-run`, wiring **all three stages**. This repo's `.pre-commit-config.yaml` has no
|
||
`default_install_hook_types`, so a plain install silently skips `commit-msg` (Conventional Commits)
|
||
and `pre-push` (everything below).
|
||
|
||
The pre-push command reports **10** hooks, not 8. The extra two are pre-commit's own `meta` hooks,
|
||
`check-hooks-apply` and `check-useless-excludes`: they declare no `stages:`, so they run at every
|
||
stage including this one. Both are declared in this repo's `.pre-commit-config.yaml` like everything
|
||
else — what separates them is `repo: meta` (pre-commit's own built-ins) from `repo: local`. Eight
|
||
is the count of hooks this repo authors itself, and `--hook-stage pre-push --all-files` is a full
|
||
rehearsal of all eight. A PR merged through Gitea's merge button runs none of them: 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 one ref.
|
||
In pre-commit 4.6.1, `_pre_push_ns` in `hook_impl.py` skips delete lines and returns on the first
|
||
remaining ref whose remote sha is non-zero and present locally; a ref whose remote sha is zero or
|
||
unknown locally is returned only if it has commits that no remote-tracking ref of that remote has.
|
||
The one hook that reads the pushed ref, `check-skill-version-bump`, therefore checks only that
|
||
ref, and the others are pushed unchecked.
|
||
When that ref's unpushed history reaches a root commit, pre-commit runs with all files and sets no
|
||
`PRE_COMMIT_TO_REF`, so `check-skill-version-bump` checks `HEAD`, which is the pushed ref only if it
|
||
is checked out. Push one ref at a time when the gate matters.
|
||
|
||
## The pre-push gate
|
||
|
||
Eight hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in.
|
||
|
||
**Core checks**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `run-tests` | `bash tests/run-tests.sh --strict` — the whole suite, skips fatal (see [Tests](#tests)) |
|
||
|
||
**Generated-content drift gates**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `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: 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**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `check-apm-agents-valid` | runs `factory-audit`'s `validate.sh` over every real `plugins/*/.apm/agents/*.agent.md` (see [Agent files](#agent-files-take-the-description-gates-not-the-body-gate)) |
|
||
|
||
**apm's own gates**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `apm-audit-ci` | `apm audit --ci` once per manifest — root plus each of the six plugin packages |
|
||
| `apm-pack-check-clean` | `apm pack --check-versions --check-clean --dry-run` — the compiled marketplace still matches what `apm.yml` + `.apm/` would generate, and per-package versions agree with the `per_package` strategy |
|
||
|
||
**Host validators** (needs the `claude` CLI on PATH)
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `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 both the merge-base's and `main`'s tip's (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
|
||
the push dies with an unhelpful "command not found". Install with `apm-install`, or
|
||
`curl -sSL https://aka.ms/apm-unix | sh`; verify with `apm --version`.
|
||
|
||
### `check-skill-version-bump`
|
||
|
||
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.
|
||
|
||
- **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-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-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`
|
||
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 each 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, and so does a skill directory left without its `SKILL.md`. Each part is ASCII
|
||
digits, at most nine of them, with no leading zero (`1.0.08` is malformed) — the same shape
|
||
`skill-size-check` enforces, so a Unicode digit, an overflowing part or an octal-looking part
|
||
cannot pass as a bump. A leading UTF-8 BOM is ignored. A baseline with no valid version accepts
|
||
any valid version. Skills absent at both baselines (new, renamed, merged) or at the pushed ref
|
||
(deleted, or replaced by a symlink) are exempt. A file moved between skills counts as a change
|
||
to both: renames are diffed as delete plus add. A mode-only change counts too.
|
||
- **It also fails closed on read errors:** a pushed ref that does not resolve to a commit
|
||
(including a tag on a tree), or a `SKILL.md` that the tree names but `git show` or `python3`
|
||
cannot read, stops the push with a read-failure message rather than being reported as a missing
|
||
version or treated as an absent skill. Presence is read from the tree, so a blob missing from a
|
||
corrupt or partial clone cannot make a skill look new. 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.
|
||
- **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:** a PR merged through Gitea's merge button
|
||
runs no local hook; and a multi-ref push checks only the one ref pre-commit selects, and a push
|
||
reaching a root commit gets no `PRE_COMMIT_TO_REF`, so `HEAD` is checked (see
|
||
[Running the gates](#running-the-gates)).
|
||
- **An all-zeros `PRE_COMMIT_TO_REF` (a branch delete) exits 0.** The branch is defensive:
|
||
pre-commit 4.6.1 skips delete lines before it sets the variable.
|
||
|
||
## Skill and agent context gates (ADR-0020)
|
||
|
||
The `skill-size-check` pre-commit hook, scoped to `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$`,
|
||
runs `scripts/skill-size-check.sh`. Besides the ADR-0020
|
||
gates below, it also asserts required frontmatter is present: `name`, a non-empty `description`, and
|
||
a `metadata.version` matching three-part semver (`1.0.0`) — folded in from a formerly standalone
|
||
`skill-frontmatter` hook that parsed the same fields with a shell script.
|
||
|
||
**Two things fall outside that scope, both deliberately.** The `[^/]+/SKILL\.md$` tail admits only a
|
||
`SKILL.md` sitting directly in a skill directory under `.apm/skills/`:
|
||
|
||
- the `plugins/kyberforge/docs/research/examples/` reference skills, which are vendored upstream
|
||
corpus and not this repo's to gate;
|
||
- `plugins/kyberforge/.apm/skills/skill-author/assets/templates/SKILL.md` — inside `.apm/skills/`,
|
||
but two directories deeper. It is the `FILL IN:` scaffold `skill-author` copies, so its
|
||
`description: >` is a comment block rather than a description and every ADR-0020 measurement over
|
||
it would be meaningless. A reader adjusting the pattern needs to know it is there.
|
||
|
||
Everything else it matches exactly, with nothing over- or under-caught. Re-derive both halves:
|
||
|
||
```
|
||
git ls-files | grep -cE '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$' # the real skills
|
||
git ls-files | grep -E '^plugins/[^/]+/\.apm/skills/.*SKILL\.md$' \
|
||
| grep -vE '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$' # the scaffold only
|
||
```
|
||
|
||
The first count equals the number of skill directories (`ls -d plugins/*/.apm/skills/*/ | wc -l`);
|
||
the second returns exactly the template. Every other tracked `SKILL.md` in the tree is one of the
|
||
four vendored `plugins/kyberforge/docs/research/examples/skill-write/` corpus files, excluded by the
|
||
`.apm/skills/` segment — the first of the two deliberate exclusions above.
|
||
|
||
### Two independent gate families, neither replaced the other
|
||
|
||
**Family 1 — agentskills.io spec backstop** (unchanged, conformance not quality):
|
||
|
||
| Constant | Value | Measured over |
|
||
|---|---|---|
|
||
| `MAX_LINES` | 500 | whole file, **frontmatter included** |
|
||
| `MAX_WORDS` | 2,770 | whole file, **frontmatter included** |
|
||
|
||
**Family 2 — ADR-0020 context budget** (measured differently, on purpose):
|
||
|
||
| Check | SUGGESTION | FAIL | Measured over |
|
||
|---|---|---|---|
|
||
| `description` characters | 250 | 400 | the YAML-**folded** value |
|
||
| body words | 600 | 900 | **body only** — everything after the frontmatter's closing `---` |
|
||
|
||
Plus two hard FAILs with no suggestion tier:
|
||
|
||
- **A missing, valueless or `null` `description:`.** Not a skip. The description is the one field
|
||
preloaded into every session, so a gate that declines to measure it reports green. (This is not
|
||
hypothetical: `description:` with no value followed by `model: sonnet` let a line regex capture the
|
||
*next* key, which looked non-empty, so the "missing or empty" branch never fired and every gate
|
||
below early-returned on the genuinely empty folded value — exit 0, zero output, on a blocking gate.)
|
||
- **Every `references/<file>.md` a body names must exist** on disk. A dispatch table pointing at a
|
||
file that was never written is a silently dead branch, and nothing else in the gate/audit/vale
|
||
stack notices it.
|
||
|
||
A file can sit well inside one family and fail the other. 2,770 whole-file words is a conformance
|
||
backstop; 900 body-only words is a quality gate. Conflating them is what produced the current state.
|
||
|
||
### An unresolved routing target is not automatically a FAIL
|
||
|
||
A boundary-clause target that resolves to no skill or agent has **three** possible verdicts, not one
|
||
(`unresolved_targets()` in `scripts/skill-size-check.sh`):
|
||
|
||
| Verdict | When |
|
||
|---|---|
|
||
| **SUGGESTION** — the default | the target does not resolve and neither promotion condition below holds |
|
||
| **blocking ERROR** | the target is written in **route notation** — `/name` for any name, or any arrow form (a bare `-> name` only when the name is hyphenated, a backticked `` -> `name` `` for any — see the gap below); **or** it is a bare **terminal** name (not a compound modifier) **corroborated** by another target in the same sentence that *does* resolve |
|
||
| **INFO, "DID NOT RUN"** | no skill universe could be determined for the path at all — the targets are named and left unchecked, exit 0 |
|
||
|
||
The default is deliberately soft because a hyphenated word in a boundary clause is as likely to be a
|
||
tool, a file format or an English compound as a route: "pre-commit hooks" is prose about a tool and
|
||
never reaches the check at all, being a compound modifier rather than a terminal name. The
|
||
SUGGESTION text says how to opt in — write it as `/name` or `-> name` and it gets checked properly.
|
||
|
||
**The two promotion conditions are not symmetric, and the order matters.** `_add()` decides
|
||
**notation first**: when the name is written `/name`, or reached through any arrow form, the target
|
||
is marked error-eligible there and the terminal test is never run. Terminality gates only the *bare*
|
||
path — a name in prose earns its error from corroboration, and a compound modifier can never dangle.
|
||
Reading the row as "terminal AND (notation OR corroborated)" gets the notation half backwards: it
|
||
predicts that `` … Do not use for Y — use /no-such-skill afterwards. `` is a SUGGESTION, because
|
||
`afterwards` is a follower outside `FOLLOWER_OK`. It exits 1. That was the defect — `-> name` reached
|
||
`_add()` with `strict=True` from both its call sites and `/name` did not, so the one spelling
|
||
ADR-0020 offers an author who wants a route checked unconditionally was the one spelling a stray
|
||
follower could silence.
|
||
|
||
**Known gap: a BARE arrow target must be hyphenated.** Target extraction is built on `NAME_HYPH` in
|
||
`scripts/skill-size-check.sh`, which requires at least one hyphen, and `ARROW_BOUNDARY` inherits
|
||
that. So `Not X -> gitea-prs` is extracted and checked, while `Not X -> triage` yields no target.
|
||
The exclusion is deliberate, not an oversight: `research`, `triage`, `forge`, `prototype` and `tdd`
|
||
are all real skill names *and* ordinary English, so a bare single-word rule would flag most of the
|
||
corpus. The marked spellings carry no such restriction — `` `triage` `` and `/triage` are both
|
||
extracted — and are the forms to prefer. **Both arrow spellings are recognised:** `ARROW_MARKED`,
|
||
`ARROW_BOUNDARY` and `BOUNDARY_ARROW` are each built from `(?:->|→)`, so the unicode arrow `→`
|
||
behaves exactly like `->` in every case below. Cite these constants by symbol name, never by line
|
||
number: the script moves often enough that a pinned line lands a reader in an unrelated comment
|
||
block and reads as plausible.
|
||
|
||
**The gap is no longer silent.** It used to be exactly that — no ERROR, no SUGGESTION, exit 0 — which
|
||
made the dangling-target SUGGESTION's own advice unsafe for a single-word skill: taking it silenced
|
||
the finding instead of checking it. `boundary_clause_status()` now separates the case out and
|
||
reports it as `unparsed` (see below), naming the parse failure and the two spellings that fix it.
|
||
The target is still not *resolved*; the author is now told so rather than left with a green gate.
|
||
`tests/test-adr0020-targets.sh` covers both directions (`arrow-single-word-target` and the silent
|
||
control `arrow-single-word-marked`).
|
||
|
||
Corroboration is what makes the soft default safe: a sentence whose *other* target resolves is
|
||
demonstrably a routing sentence, so a sibling that does not resolve is a typo rather than a noun, and
|
||
gets promoted.
|
||
|
||
### Target resolution walk
|
||
|
||
Resolution walks up **from the file being checked** — never from the script's own location. Deriving
|
||
it from `${BASH_SOURCE}` leaked holocron's own skill universe into every consumer repo running the
|
||
hook through pre-commit, so a consumer skill routing to a holocron skill (`skill-audit` at the time,
|
||
now `factory-audit`) resolved against a plugin it had never installed.
|
||
|
||
The walk finds an **authoring root**: the nearest ancestor holding `plugins/*/.apm/skills` or
|
||
`plugins/*/.apm/agents`, falling back to the nearest ancestor holding `.git`. **Two passes, not one
|
||
interleaved walk**, so a nested `.git` (a submodule, a sub-package worktree) cannot beat a real
|
||
monorepo root further up.
|
||
|
||
The universe is then:
|
||
|
||
1. every skill and agent under `<root>/plugins/*/` — sibling plugins resolve, which is what a
|
||
monorepo means;
|
||
2. the checked file's own apm package;
|
||
3. the packages that package declares in **its own** `apm.yml` `dependencies.apm`.
|
||
|
||
The **root** manifest's `dependencies:` block is not read, and no plugin here declares a cross-plugin
|
||
apm dependency — none needs to.
|
||
|
||
Deployed `.claude/` / `.agents/` trees are consulted **only** when the walk found no plugin monorepo
|
||
root, whether it landed on a bare `.git` ancestor or on nothing at all. That is the consumer case.
|
||
|
||
**The gate keys on which of the two passes matched, never on whether the root contributed a new
|
||
name.** A name-count delta looks equivalent and is not: `_collect_authoring_root()` re-collects the
|
||
checked file's own plugin, whose names the earlier steps already added, so a single-plugin monorepo
|
||
shows a delta of zero and would wrongly reach for the deployed trees — including the user's global
|
||
`~/.claude/skills`, making the verdict depend on what happens to be installed.
|
||
|
||
Why it matters: those trees are gitignored `apm install` output, present only on a machine that has
|
||
run it. Four cross-plugin targets here (`gitea-branches` → `git-branches`, `gitea-branches` →
|
||
`git-history`, `gitea-issues` → `git-branches`, `gitea-workflow` → `git-workflow`) once resolved
|
||
through `.claude/skills/` alone, so **the same commit measured 2 dangling targets on a developer
|
||
machine and 6 on a fresh clone**. A gate shipping hot with no baseline cannot give two answers.
|
||
|
||
Verified: running the hook over a tree holding only `plugins/` and the root `apm.yml`, with no
|
||
`.claude/` or `.agents/` anywhere, produces findings identical to the working tree — confirming the
|
||
two trees agree on the current corpus, independent of what happens to be installed locally.
|
||
|
||
### Boundary-clause detection: three outcomes, not two
|
||
|
||
`boundary_clause_status()` returns one of three values, and the two findings get separate messages:
|
||
|
||
| Status | When | Reported as |
|
||
|---|---|---|
|
||
| `present` | a prose marker (`do not`, `instead`, `rather than`, `not for`) or an arrow clause was found | nothing |
|
||
| `absent` | neither was found | SUGGESTION: add a boundary clause, in either form |
|
||
| `unparsed` | an arrow clause was found and **no target could be read out of it** | SUGGESTION: the clause is present — this is a *parse* failure, not a missing clause |
|
||
|
||
The third had to be split out. Collapsing it into `absent` is a **wrong** finding, not a strict one:
|
||
it sends the author to add a clause that is already there. Three of them instead reworded a correct
|
||
clause until the regex accepted it, one stripping the very filename that discriminates the skill
|
||
from its neighbour (**#110**).
|
||
|
||
`unparsed` is narrow and certain on purpose. It fires only on the arrow form, which *always* names a
|
||
target, so zero targets means the name is written in a shape the extractor cannot see — in practice
|
||
a bare single-word target, per the known gap above, and the message says to write it `` `name` `` or
|
||
`/name`. A **prose** clause yielding no target is not reported at all: "Do not use for anything else"
|
||
is a complete and legitimate boundary clause that names nowhere to go.
|
||
|
||
**One arrow, one target.** An arrow clause naming two or more targets draws its own SUGGESTION,
|
||
quoting both names and asking for a split, because only the first is ever resolved: the conjunction
|
||
continuation (`CONT_MARKED` / `CONT_ANY`) is wired to the prose route verbs and never to arrows. So
|
||
`Not X -> a or b` resolved `a`, left `b` resolved by nothing and reported by nothing, and then let
|
||
the audit print "1 of 1 boundary target(s) resolve" on a clause naming two — a gate under-reporting
|
||
its own coverage, which is the one failure mode ADR-0020 says a gate must not have (**#107**). The
|
||
clause is **rejected rather than the arrow scan extended**: extending it would widen the resolver's
|
||
deliberately conservative false-positive tuning across every arrow in the corpus, where splitting
|
||
costs the author one full stop. The convention is one arrow per target — `Not X -> a. Not Y -> b.` —
|
||
already what every retrofitted `gitea-*` skill does in practice, now stated in
|
||
`skill-author`'s `references/contract.md` instead of being folklore.
|
||
|
||
**Dotted filenames in a boundary clause now parse.** `CLAUSE_BODY` — what may sit between `Not` and
|
||
the arrow — used to be `[^.;]`, a class that cannot cross a `.`, so every clause naming a dotted
|
||
filename between the two (`AGENTS.md`, `.vale.ini`, `.pre-commit-config.yaml`) was invisible to both
|
||
`BOUNDARY_ARROW` and `ARROW_BOUNDARY`. The two resulting failures were different sizes (**#110**):
|
||
|
||
- with a **backticked** target the clause was *misdiagnosed*. The backtick sweep still extracted the
|
||
target, so the route was checked, but the gate reported "no boundary clause" on a clause that was
|
||
present and working. That is the misdiagnosis the three rewordings above came from.
|
||
- with a **bare** target the clause was *unchecked*. `ARROW_BOUNDARY` is the only extractor for a
|
||
bare arrow target, so `Not AGENTS.md -> no-such-skill` produced no target, no dangling report and
|
||
no missing-clause SUGGESTION. Silence, not noise — the worse of the two.
|
||
|
||
`CLAUSE_BODY` is now `(?:[^.;]|\.(?=\S))`: a dot inside a filename is followed by a non-space, a
|
||
sentence-ending dot by whitespace or end of string, so the class crosses `AGENTS.md` and still stops
|
||
at a real sentence end. **Read the second bullet forward as well as back:** a bare target sitting
|
||
after a dotted filename is now extracted, resolved, and a blocking ERROR when it dangles, where the
|
||
same clause used to pass unchecked in silence.
|
||
|
||
### SUGGESTION-only checks
|
||
|
||
Deterministic to measure, judgment to act on:
|
||
|
||
- a description with **no boundary clause at all** (`absent`);
|
||
- an **arrow clause whose target could not be read** (`unparsed`);
|
||
- an **arrow clause naming more than one target**;
|
||
- a `## Gotchas` section with **more than five entries**;
|
||
- a `## Gotchas` section over **25% of the body**.
|
||
|
||
### Hand-invoked skills are exempt from the routing rules, and only those
|
||
|
||
A skill or agent whose frontmatter carries `disable-model-invocation: true` skips three checks:
|
||
|
||
- the boundary-clause check, `absent` and `unparsed` alike;
|
||
- the multi-target arrow check;
|
||
- the 250-character description **target** (`hand_invoked()` in `scripts/skill-size-check.sh`).
|
||
|
||
It keeps the 400-character description FAIL and **both** body word tiers, and if its description
|
||
does happen to name a target, that target is still resolved and can still dangle.
|
||
|
||
Why the exemption is right: `disable-model-invocation: true` removes the skill from the
|
||
model-visible listing entirely — it is not preloaded, and the Skill tool refuses to call it — so its
|
||
description is never matched against user intent. ADR-0020 and `skill-author`'s contract therefore
|
||
give such a skill **one plain human-facing sentence**: no trigger list, no boundary clause. No
|
||
validator knew the field existed (**#108**), so the boundary-clause SUGGESTION fired on exactly the
|
||
shape the contract mandates, and its remedy — "so the router knows where NOT to send this skill" —
|
||
was addressed to a router that cannot see the skill at all. An author who followed the advice made
|
||
the file worse. There is no router to inform.
|
||
|
||
The half that does **not** lift is the point. The body is still loaded on invocation and still
|
||
competes with the caller's live conversation, so neither body tier moves. The 400-character ceiling
|
||
stands too: a hand-invoked description is not preloaded, but it is still the one line the user reads
|
||
when choosing from the `/` menu, and that ceiling is an outlier stop rather than a routing-quality
|
||
budget — which is precisely why the 250-character target is the tier that lifts.
|
||
|
||
The field is read as a **boolean**, not as a mention of the key. PyYAML already resolves the
|
||
unquoted YAML 1.1 booleans, so the extra handling catches a quoted `"true"`, which a host reads as
|
||
truthy; `disable-model-invocation: false` is the model-invoked case written out longhand and buys
|
||
nothing. A frontmatter parse failure returns false rather than raising — the flag is a *modifier* on
|
||
other checks, and `description_value()` on the same text already reports the broken frontmatter, so
|
||
raising here would diagnose one file twice two different ways.
|
||
|
||
`caveman` and `zoom-out` are the two carriers here. `tests/test-skill-size-check.sh` pins both
|
||
halves — what the carve-out lifts, each with a flag-removed control, and what it must not.
|
||
|
||
### `verbose: true` is load-bearing
|
||
|
||
The hook is declared `verbose: true` so the SUGGESTION tier is audible. pre-commit prints nothing at
|
||
all for a passing hook, and a SUGGESTION deliberately does not fail — without verbose every
|
||
suggestion is swallowed, which is exactly the invisibility ADR-0013 records for Vale warnings.
|
||
ADR-0020's preload arithmetic depends on it: writing to the 400-char FAIL delivers roughly half the
|
||
cut that writing to the 250-char SUGGESTION does, so the intended saving depends entirely on that
|
||
tier being visible. The numbers, and the measurement method behind them, are not restated here —
|
||
they live in ADR-0020's Consequences section, under "A ceiling does not produce an average", whose
|
||
figures are pinned to the base commit the decision was taken on (`f9b919d`). Quoting them here would
|
||
just create a second copy to go stale. It costs nothing on a clean file — the script prints only
|
||
findings.
|
||
|
||
### Duplicated constants
|
||
|
||
`factory-audit` holds a second copy of the four ADR-0020 constants
|
||
(`DESC_SUGGEST_CHARS` / `DESC_MAX_CHARS` / `BODY_SUGGEST_WORDS` / `BODY_MAX_WORDS`). They are not in
|
||
its `validate.sh`, which carries none of them: they live in the mode libraries it sources —
|
||
`scripts/lib-checks-skill.sh:313-316` carries all four, and `scripts/lib-checks-agent.sh:164-165`
|
||
carries the two description constants only. That split is the contract stated directly: the two
|
||
description constants apply to both artifact types `factory-audit` handles, the two body constants
|
||
only to skills. They are copied rather than imported because a skill's files may not reach outside that skill's own
|
||
directory (the self-contained constraint in `docs/spec/architecture.md`, "Plugin model"), and
|
||
`scripts/skill-size-check.sh` does not ship with the plugin. `tests/test-skill-size-check.sh` asserts the copies agree,
|
||
so drift fails CI rather than silently letting an audit bless a skill the commit hook then rejects.
|
||
|
||
**The shared boundary resolver is one copy** (ADR-0025, then 2026-09-16). It lives between the
|
||
`BEGIN`/`END ADR-0020 SHARED BOUNDARY RESOLVER` markers in `factory-audit/scripts/lib-boundary-resolver.sh`.
|
||
ADR-0025 collapsed the two copies inside `skill-audit`'s and `agent-audit`'s `validate.sh` into that
|
||
file. `scripts/skill-size-check.sh` kept an embedded, byte-identical third copy while it was also
|
||
exported through `.pre-commit-hooks.yaml`, whose consumers could not reach a file inside the plugin.
|
||
`4de5b6b` retired that export (ADR-0014), so the hook now sources the library by path and fails closed
|
||
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, 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,
|
||
with the agent path running no resolver or some other one. It is now a **per-arm structural check** —
|
||
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 → 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 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.
|
||
|
||
### `python3` and PyYAML are hard requirements
|
||
|
||
Both, and neither is a best-effort accelerator.
|
||
|
||
`python3` because the script measures the **folded** `description` value. Most descriptions here are
|
||
`>`-block scalars, so a regex over the raw lines measures indentation and newlines instead of the
|
||
value. Missing it fails the hook with an install pointer rather than skipping the ADR-0020 checks,
|
||
which would be a vacuous green — a gate that reports success without having checked anything. In practice it is already present — pre-commit is itself a Python
|
||
application.
|
||
|
||
**PyYAML** because the hand-rolled fallback frontmatter reader has been **removed deliberately**. It
|
||
disagreed with a real parser across the FAIL boundary — one corpus description measured 270
|
||
characters parsed and 412 unparsed — and a quoted `"description"` key or an explicit
|
||
`description: null` returned empty from it, silently skipping the description *and* routing checks. A
|
||
reader that mis-parses an unfamiliar scalar shape reports a clean pass on a file it never measured,
|
||
which is the exact vacuous-green failure the `python3` check exists to avoid. `pip install pyyaml`
|
||
(or `python3 -m pip install PyYAML`, or the distro's `python3-yaml`) if the hook reports it missing.
|
||
|
||
**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
|
||
|
||
`check-apm-agents-valid` runs `factory-audit`'s `validate.sh` over every real
|
||
`plugins/*/.apm/agents/*.agent.md`. It derives its expected file set from `git ls-files` — the pattern
|
||
`tests/run-bats.sh` established — so an agent file deleted from the worktree but still tracked fails
|
||
the run, and **discovering zero agent files is an error, not a pass**. An untracked *new* agent file
|
||
is still validated: the derivation is one-directional on purpose, so uncommitted work is not blocked
|
||
but also cannot bypass the gate.
|
||
|
||
The hook exists because `validate.sh` was previously exercised only by `check-scope-walkup-sync`,
|
||
against synthetic `mktemp` fixtures — it had never run against the agent files it governs. That is
|
||
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 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
|
||
ADR rather than fixing an inconsistency. The merge did not change this: the validator auto-detects
|
||
the target type, and the body gate applies on the skill path only.
|
||
|
||
**Be precise about the scope of that guarantee: it holds for the *validator*, not for the shared
|
||
script.** `scripts/skill-size-check.sh` applies its body gate to whatever path it is handed, and
|
||
|
||
```
|
||
bash scripts/skill-size-check.sh plugins/*/.apm/agents/*.agent.md
|
||
```
|
||
|
||
exits 1 today with 900-word body FAILs on `git-orchestrate` and `gitea-orchestrate`. (Counts are
|
||
deliberately not pinned here — agent bodies are edited like any other file, and a figure in this
|
||
paragraph goes stale the moment one is trimmed. Run the command.) Agent files escape only because
|
||
the hook definitions filter on `SKILL.md`
|
||
— a file-pattern accident that happens to implement the design, not the design itself. **Do not
|
||
"extend" that hook's `files:` pattern to cover agents** on the assumption that the script already
|
||
knows the difference; doing so silently enforces a gate ADR-0020 declines to set.
|
||
|
||
### The three exit tiers of `factory-audit`'s validators
|
||
|
||
`validate.sh` and `validate-provenance.sh` use the exit code to say **whether an audit happened**,
|
||
not only what it found. The skill's flow references tell the auditor to surface a non-zero run's
|
||
output verbatim as real findings, so the distinction is load-bearing rather than cosmetic:
|
||
|
||
| Exit | Means |
|
||
|---|---|
|
||
| **0** | audited, and clean |
|
||
| **1** | audited, and there are findings — or the target is present but cannot be opened |
|
||
| **2** | **nothing was audited.** The checks never ran |
|
||
|
||
Exit 2 covers: no arguments at all; a target that does not exist, with its own "does not exist"
|
||
message; a target matching neither the skill nor the agent shape; a missing or unreadable `lib-*.sh`
|
||
beside the entry script; and a missing `python3` or PyYAML. Every one of those used to exit 1, so
|
||
an install problem or a typo'd path arrived at the auditor as findings about the artifact. A missing
|
||
target was the sharpest case: only the directory branch stats the target, so a typo'd `*.agent.md`
|
||
path was classified on name alone, handed to `python3`, and came back as a FAIL about a file that was
|
||
never there.
|
||
|
||
**A dangling symlink or a symlink loop stays exit 1, deliberately.** Both are false to `-e` and true
|
||
to `-L`, and the existence guard tests both: something *is* at that path, it just cannot be opened,
|
||
and "exists but unreadable" is a real finding the check suite reports as a FAIL naming the file.
|
||
Catching them in the existence guard would replace that FAIL with a false "does not exist".
|
||
|
||
`--help` is exit 0 and now works with a library missing. `validate-provenance.sh` sourced both
|
||
provenance libraries at the top of the file, so a partial install turned the one command that
|
||
explains how to use the script into an exit 2; the libraries are now loaded only when a mode's half
|
||
of the usage is actually printed, and when one is gone that half says so instead of the whole command
|
||
failing. It also separates an **empty** positional from **no** positional — `validate-provenance.sh
|
||
""`, an unquoted variable that expanded to nothing, used to be reported as "only flags were given",
|
||
which sent the reader after a flag they never typed. Both are exit 2, with different messages.
|
||
|
||
**All three entry scripts are CDPATH-safe.** `cd` **prints** the directory whenever CDPATH supplied
|
||
it, so a bare `cd` on a path starting with neither `/` nor `.` can both emit an extra line into
|
||
whatever captures it *and* resolve to an unrelated tree. Every such site now does
|
||
`CDPATH='' cd -- …`: `SCRIPT_DIR` and the target's parent-directory name in `validate.sh` and
|
||
`validate-provenance.sh`, and **both** sites in `vale-wrap.sh` — its default `--config` resolution,
|
||
and the directory-mirror walk below. The two bare `cd`s left in `vale-wrap.sh` take absolute paths
|
||
(`mktemp -d` output, and the mirror root derived from it), which CDPATH is never consulted for.
|
||
|
||
The second of `vale-wrap.sh`'s two sites is not an asset lookup: its directory mirroring runs
|
||
`cd "$arg" && find -L . … -print0` on a *path argument*, which is relative whenever the caller passed
|
||
a relative one. With an exported CDPATH holding a same-named directory, the `cd` emitted the decoy's
|
||
path into the `-print0` stream and the mirror was built from the decoy's files. It is cleared the
|
||
same way now. Only the `cd` changed: the walk still mirrors the whole tree, prunes `.git`, and
|
||
follows symlinks with `find -L` because vale does.
|
||
|
||
**That one was latent, not a live gate defect.** Neither published Vale hook reaches it: both filter
|
||
on `files:` patterns that match single markdown files, and only the `-d "$arg"` branch mirrors a
|
||
directory. The exposed caller is the hand-invoked `vale-wrap.sh <dir>`.
|
||
|
||
## Current retrofit status
|
||
|
||
The ADR-0020 gates ship hot, with no baseline file — a shrinking baseline was considered and
|
||
rejected. The corpus is currently clean on both: 0 of 38 descriptions/bodies exceed their FAIL tier,
|
||
0 dangling targets, 0 `Kyberforge.CompositionNote` (Vale) errors. History: issue #99.
|
||
|
||
Nothing is grandfathered — a new skill, or an edit that crosses a FAIL tier, is blocked on first
|
||
commit. SUGGESTION counts are not pinned here; they move with every edit. Measure and check both
|
||
gates before starting work on a skill:
|
||
|
||
```
|
||
bash scripts/skill-size-check.sh plugins/*/.apm/skills/*/SKILL.md | grep -c '^SUGGESTION'
|
||
pre-commit run --all-files # size AND Vale — skill-size-check alone can pass while Vale still blocks
|
||
```
|
||
|
||
## The `rtk` prefix gate (ADR-0023)
|
||
|
||
`check-rtk-prefix` is a `repo: local` pre-commit hook running `scripts/check-rtk-prefix.sh` over
|
||
`^plugins/[^/]+/\.apm/(skills/.*\.md|agents/.*\.agent\.md)$`, with `README.md` excluded. It enforces
|
||
**ADR-0023 clause 1 and nothing else**: an executable, instructed local git command in plugin skill
|
||
or agent content is written `rtk git`.
|
||
|
||
It is wider in file scope than the ADR-0020 hooks — every markdown file under a plugin's
|
||
`.apm/skills/` and `.apm/agents/`, not `SKILL.md` alone — because the rule it enforces is about
|
||
commands an agent runs, and most of those live in `references/`, which the ADR-0020 gates do not
|
||
reach ([the `references/` blind spot](#the-blind-spot-references-is-unlinted-for-two-independent-reasons)).
|
||
|
||
### What it can decide, and what it declines to
|
||
|
||
ADR-0023 has three clauses and only the first is a pattern:
|
||
|
||
| Clause | Rule | Gated |
|
||
|---|---|---|
|
||
| 1 | executable + instructed → `rtk git` | yes |
|
||
| 2 | illustrative / referential → bare `git` | no — undecidable |
|
||
| 3 | machine-parsed or interactive → bare `git` | no — opt-out marker |
|
||
|
||
Clause 2 is a judgement about what a sentence is *doing*. "Run `git switch <branch>`" and "`git
|
||
switch` refuses rather than clobbering local edits" are the same token sequence. A gate that guessed
|
||
would fire on correct prose, and **a gate that fires on correct content gets added to `SKIP`** —
|
||
which disarms clause 1 along with it. So the hook looks only at the two contexts where a `git`
|
||
mention is unambiguously an instruction to execute:
|
||
|
||
- a line inside a fenced code block whose info string names a shell — `bash`, `sh`, `shell`, `zsh`,
|
||
`console`, `shell-session`. Fences tagged `text`, `yaml`, `json`, or tagged with nothing, are **not**
|
||
checked;
|
||
- the **opening** backticked span of a "Run" column cell in a markdown dispatch table, and only the
|
||
opening span.
|
||
|
||
That last narrowing is not fussiness. A Run cell routinely carries a command followed by prose about
|
||
it, and the prose is clause 2. `git-worktrees/SKILL.md` has both shapes on adjacent rows — one cell
|
||
reading `` `rtk git worktree add --track …` `` — always correct. `` `git worktree add <path>
|
||
<branch>` `` expands to exactly this (instruction, then reference), and a `**Never** …` row whose Run
|
||
cell is entirely explanation containing a bare `git push`. Checking every backticked span flags both;
|
||
checking only a leading span flags neither, and still catches the ordinary
|
||
`` | List | `git worktree list -v` | `` case the gate exists for.
|
||
|
||
### The clause-3 opt-out
|
||
|
||
A command that is deliberately bare — because rtk rewrites the output the skill parses, or because
|
||
the command is interactive — is exempted by putting the literal string `ADR-0023` **on the same
|
||
line**: in a shell comment for a code line, in the cell text for a table row.
|
||
|
||
Per line, never per block. A fenced procedure routinely mixes `rtk git` steps with one deliberately
|
||
bare command (`git-remotes/references/push.md` does exactly that), and a block-level marker would
|
||
silently disarm every checked line around the marked one. The cost is a repeated `# bare per
|
||
ADR-0023` in the three blocks of `git-log-format.md` where every line is deliberately bare; that
|
||
repetition is the price of the marked line being the only line the marker speaks for.
|
||
|
||
The marker is a plain substring match, so a line that mentions `ADR-0023` for an unrelated reason is
|
||
also exempt. Accepted deliberately: the marker records an author's opt-out, it is not a security
|
||
boundary, and a stricter form would only move the same trust to a different string.
|
||
|
||
### What it deliberately does not cover
|
||
|
||
- **Clause 2.** Nothing checks that an illustrative mention stayed bare. A sweep that re-prefixes a
|
||
referential `git` passes this gate. The inline reasons ADR-0023 requires on clause-3 sites are the
|
||
only defence, and they are prose.
|
||
- **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.** 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
|
||
quotes, so a git command inside a quoted argument is decided by accident.
|
||
`rtk git submodule foreach 'git pull origin main || :'` passes because the segment holding the
|
||
inner command begins with `rtk` — the right answer for the wrong reason. Write
|
||
`foreach 'git a; git b'` and the second inner command is a false positive needing the marker.
|
||
ADR-0023 records this shape as one the rule itself does not decide.
|
||
- **Non-git commands.** Only `git` is checked. `rtk` fronts `gh`, `docker`, `kubectl` and others; no
|
||
gate covers those, and the corpus does not currently instruct them.
|
||
|
||
`tests/test-check-rtk-prefix.sh` pins all of it, including the false-positive cases. Its first case
|
||
reconstructs the plugin corpus as it stood on `main` before the #113 sweep and asserts the gate
|
||
fails there with at least 20 findings, one of them the `gitea-*` `git remote get-url origin` drift
|
||
the sweep missed — a gate that only passes on the already-fixed tree proves nothing about the drift
|
||
it was written for.
|
||
|
||
## Vale
|
||
|
||
Install the `vale` binary — `brew install vale` (macOS), `snap install vale` (Linux),
|
||
`choco install vale` (Windows), or see <https://vale.sh/docs/vale-cli/installation/>. No `vale sync`
|
||
is needed: the `Kyberforge` styles are **committed** under
|
||
`plugins/kyberforge/.apm/skills/factory-audit/assets/vale/styles/`, not downloaded packages
|
||
(ADR-0014).
|
||
|
||
### One copy, one config
|
||
|
||
Wiring Vale as a deterministic prefilter for `factory-audit`'s Description dimension (motivation:
|
||
issue #84) is repo-specific, not part of the generic `lint` plugin, so it does not live in
|
||
`plugins/lint/` — and per ADR-0014 it no longer lives at the repo root either. It lives **once**,
|
||
under `plugins/kyberforge/.apm/skills/factory-audit/assets/vale/`, carrying both the `Kyberforge`
|
||
and `KyberforgeCopilot` styles and a single `.vale.ini` with all three glob sections:
|
||
`[**/SKILL.md]`, `[**/agents/*.md]`, `[**/*.agent.md]`.
|
||
|
||
ADR-0014 split this into two skill-scoped copies because a plugin's cache-install copies only each
|
||
skill's own files and `skill-audit` could not reach across the skill boundary into `agent-audit`'s
|
||
copy. Merging the two audit skills removed the boundary, so the copy went with it and the single-file
|
||
`.vale.ini` ADR-0014 split apart is restored (ADR-0025). `KyberforgeCopilot` stays scoped to
|
||
`.agent.md` files alone, for the Copilot-only "`Use proactively` has no effect" check.
|
||
|
||
With one copy there is nothing left to diff, so the `check-vale-style-sync` pre-push hook,
|
||
`scripts/check-vale-style-sync.sh` and `tests/test-check-vale-style-sync.sh` are deleted — one hook
|
||
off the push gate. **Read what went with it, not just what became vacuous.** The script had 17
|
||
assertion sites. ADR-0025 maps each one; the short version follows.
|
||
|
||
**Genuinely moot (6):**
|
||
|
||
- the `vale-wrap.sh` diff and the `styles/Kyberforge/` diff, which compared two copies that are now
|
||
one;
|
||
- the four hard-fail guards that located those copies (`REPO_ROOT` is a directory, the `.apm/` paths
|
||
are not stale, neither copy is missing).
|
||
|
||
Its `StylesPath` and `BasedOnStyles` checks were **not** diffs. They were per-file greps of each
|
||
`.vale.ini`, so they survive: case 0 below checks that the config loads, and case 28 checks that the
|
||
`Kyberforge` style is actually loaded.
|
||
|
||
**Rehomed or ported (11).** The largest group is the **six-row glob-coverage probe table**, which
|
||
invoked `vale --config` on one representative path per file shape. It was the only assertion
|
||
anywhere that catches a `.vale.ini` glob typo (`[**/SKILL.md]` → `[**/SKILLS.md]`), the failure mode
|
||
where every other check stays clean while Vale lints zero files. One config does not make that
|
||
impossible: a typo in any one of the three sections still 0-file-skips that shape.
|
||
|
||
**Case 0** runs before any Vale-dependent case and needs no Vale binary. It asserts that the shipped
|
||
`.vale.ini` exists and is readable, sets a `StylesPath` that resolves to a directory, and names only
|
||
styles that ship. A config that cannot load used to surface as nine generic "vale printed no summary
|
||
line" failures across cases 28–31. It now fails once, names the cause, and holds the Vale-dependent
|
||
cases back.
|
||
|
||
The probes now live in `tests/test-vale-wrap.sh` (cases 28–30), rehomed against the merged config:
|
||
one representative path per file shape, each asserted to produce a Vale scan of more than zero files
|
||
*and* a Kyberforge alert (case 28). Case 28 also checks that each probe path is in scope of a
|
||
published vale hook, and that every `.vale.ini` section has a probe row. Its Part B drops
|
||
`Kyberforge` from each section's `BasedOnStyles` in a copy and requires that section's probes to
|
||
fail as "style not loaded". Case 29 is a mutation case: it typos each section in a copy of the
|
||
assets and requires that section's isolating probes to drop to zero. Case 30 asserts that
|
||
`KyberforgeCopilot` reaches `.agent.md` files alone. Its Part B requires both an unload (dropped from
|
||
`[**/*.agent.md]`) and a leak (added to `[**/SKILL.md]`) to fail. Case **31** is the third class that went with the
|
||
script and is not a glob probe at all: the per-rule override allowlist, which pins every Kyberforge
|
||
rule at a blocking bare `YES`/`error`. It is not redundant with the probes above — those key on
|
||
`Kyberforge.VagueWording` and `KyberforgeCopilot.ProactivePhrase`, so the other four rules
|
||
(`DescriptionOpener`, `PaddingPhrase`, `SentenceOpenerThereIs`, `CompositionNote`) can each be
|
||
overridden out of `error` underneath a passing probe. That gap is closed.
|
||
|
||
**Case 32** covers the prefilter hooks' own scope in `.pre-commit-config.yaml`, with three
|
||
properties. Each vale hook's `files:` regex must still match at least one tracked file; every path it
|
||
matches must be in that hook's own artifact class; and it must match **every** tracked file of that
|
||
class under `plugins/*/.apm/`. A hook narrowed to zero files never runs, and pre-commit reports no
|
||
error. A hook narrowed to one plugin still matches files of the right class, which is why the third
|
||
property exists: narrowing `vale-audit-prefilter-skill` from `^plugins/[^/]+/...` to
|
||
`^plugins/kyberforge/...` once left 6 of 38 skills prefiltered and the whole suite green. Part B
|
||
narrows both regexes to zero files and Part C narrows both to one plugin, each in a copy of the
|
||
config, and requires Part A to fail by hook name.
|
||
|
||
Case 32 used to have a partner, case 33, which compared each hook's `files:` regex against the
|
||
published `.pre-commit-hooks.yaml`. It went with that manifest (ADR-0014's 2026-09-16 amendment),
|
||
and its one guard that did not need a second manifest, the one-plugin narrowing, is case 32's third
|
||
property.
|
||
|
||
**Case 34** asks, statically and with no Vale binary, whether the shipped `.vale.ini` could load a
|
||
style at all. Four assertions: every `[glob]` section declares a **non-empty** `BasedOnStyles`; every
|
||
style any section names resolves to a real directory under `StylesPath`; that `StylesPath` is **not
|
||
absolute**; and at least one `[glob]` section exists, so the check cannot pass vacuously on a config
|
||
with nothing in it. A section whose `BasedOnStyles` is empty is the silent case — Vale lints every
|
||
file that glob matches with no rule loaded, prints `0 errors` and exits 0. The absolute-path clause
|
||
is the one that is not obvious: an absolute `StylesPath` passes on the machine that wrote it and
|
||
hard-fails for every repo that installs `factory-audit`, which is the only reason those styles ship
|
||
with the skill at all. Part B is a mutation self-test against the same function Part A calls — it empties
|
||
each section's `BasedOnStyles` in a copy of the assets, and absolutizes `StylesPath` in another
|
||
pointed at that copy's own real `styles/` directory, and requires each to fail by name.
|
||
|
||
**It is not case 0 again.** Case 0 reads `BasedOnStyles` lines file-wide and tolerates an absolute
|
||
`StylesPath`; case 34 reads them **per `[glob]` section** and rejects one. Case 0 checks the config
|
||
loads; case 34 checks that loading it arms anything.
|
||
|
||
### What Vale owns, and what stays LLM judgment
|
||
|
||
Six rule files, six distinct rules:
|
||
|
||
| Rule | Vale scope | Bans | From |
|
||
|---|---|---|---|
|
||
| `Kyberforge.DescriptionOpener` | `text.frontmatter.description` | non-imperative openers ("This skill/agent…") | issue #84 |
|
||
| `Kyberforge.VagueWording` | `text.frontmatter.description` | vague capability wording ("helps with", "utilize", …) | issue #84 |
|
||
| `Kyberforge.PaddingPhrase` | `text` | generic "see `references/` for details" padding | issue #84 |
|
||
| `KyberforgeCopilot.ProactivePhrase` | `text.frontmatter.description` | `Use proactively` (no effect in Copilot) | issue #84 |
|
||
| `Kyberforge.SentenceOpenerThereIs` | `sentence` | "There is/are" sentence openers | ADR-0013 |
|
||
| `Kyberforge.CompositionNote` | `text.frontmatter.description` | architecture and composition prose in a description | ADR-0020 |
|
||
|
||
Vale covers the **pattern-matchable** sub-checks named in issue #84 plus, per ADR-0013, one
|
||
cherry-picked body-wide prose-pattern rule. Everything else stays LLM judgment: defaults-vs-menus,
|
||
why-rationale, the non-pattern-matchable body-discipline calls, near-miss exclusion strength, and
|
||
control calibration. New rules land directly in `styles/Kyberforge` and block immediately — there is
|
||
no trial tier.
|
||
|
||
The cherry-pick record, so it is not re-litigated:
|
||
|
||
- `Kyberforge.SentenceOpenerThereIs` **landed** — 22 held-out hits, both in-corpus hits clean
|
||
rewrites, zero suppressions needed.
|
||
- `Kyberforge.VagueQualifier` was cherry-picked and then **deleted**. 2 hits across the corpus as it
|
||
stood on 2026-08-08 (before the `.apm/` restructure): one marginal, and one unfixable false
|
||
positive — `caveman/SKILL.md` quotes `of course` as an example of filler, a mention rather than a
|
||
use — which forced the repo's only Vale suppression comments.
|
||
- `governance.md` and `CONTROLS.md` were evaluated as rule sources and **excluded**: nothing
|
||
prose-pattern-matchable to mine.
|
||
|
||
### Why every rule is `level: error`
|
||
|
||
Every alert is a FAIL, with no ignorable tier — same all-or-nothing model as shellcheck, the test
|
||
suite, and conventional-pre-commit. Graded severities do not work here: **Vale's exit code keys on
|
||
`error` alerts alone**, so a `warning` or `suggestion` rule exits 0, and pre-commit swallows a
|
||
passing hook's output. Such a rule would be invisible and would block nothing.
|
||
|
||
`MinAlertLevel` and `--minAlertLevel` are correspondingly **absent** from both `.vale.ini` files and
|
||
from the hook definitions. Under this model they are no-ops; adding one is not a missing knob.
|
||
|
||
The `verbose: true` escape hatch that makes `skill-size-check`'s SUGGESTION tier audible has no
|
||
analogue here — Vale has no tier to make audible.
|
||
|
||
### Pre-commit
|
||
|
||
Two prefilter hooks, with `.apm/`-scoped `files:` patterns:
|
||
|
||
| Hook | Pattern |
|
||
|---|---|
|
||
| `vale-audit-prefilter-skill` | `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$` |
|
||
| `vale-audit-prefilter-agent` | `^plugins/[^/]+/\.apm/agents/[^/]+\.agent\.md$` |
|
||
|
||
Only the **authoring source** triggers them. The `skill-author` scaffold template and the four
|
||
vendored research-corpus `SKILL.md` files match neither pattern (see
|
||
[Skill and agent context gates](#skill-and-agent-context-gates-adr-0020), which uses the same scope
|
||
for `skill-size-check`), so prose findings surface only when you edit a file this repo actually
|
||
authors. Without the binary the hooks fail with a bare "command not found" and no install pointer.
|
||
|
||
**Two hooks, not one combined hook — for a different reason than ADR-0014 gave.** The original
|
||
reason was mechanical: with a config per skill, a single hook could point at only one copy and would
|
||
silently 0-file-skip the other file shape (see
|
||
[A 0-file Vale run is NOT RUN](#a-0-file-vale-run-is-not-run)). One `.vale.ini` carrying all three
|
||
sections removes that constraint. The split stays anyway because the `files:` regexes still have to
|
||
differ — each hook hands Vale only the file shape it is scoped to. Both hooks name the same
|
||
plugin-bundled `factory-audit/scripts/vale-wrap.sh` through `repo: local`; there is no second root
|
||
copy.
|
||
|
||
### The `.vale.ini` globs do no scoping
|
||
|
||
The `.vale.ini`'s section globs are **path-agnostic** — `[**/SKILL.md]`, `[**/agents/*.md]` and
|
||
`[**/*.agent.md]` — and constrain filename *shape*, not
|
||
location: Vale's `*` crosses `/`. A `SKILL.md` outside `plugins/` (a project-scope
|
||
`.claude/skills/foo/SKILL.md`, say) still matches `[**/SKILL.md]` and gets linted normally.
|
||
|
||
All scoping therefore comes from the pre-commit hooks' own `files:` regexes, which pin this repo's
|
||
layout (see [Pre-commit](#pre-commit)), and from `factory-audit` passing one explicit file per
|
||
invocation — in this repo or in any repo that installs it, whatever that repo's layout.
|
||
|
||
Narrowing a `.vale.ini` glob to a `plugins/`-shaped path to "tighten" it breaks the consumer case:
|
||
`factory-audit` run against a project-scope `.claude/skills/` tree would lint nothing.
|
||
`check-vale-style-sync`'s probe set was built to catch exactly that; it moved to
|
||
`tests/test-vale-wrap.sh` with the hook's deletion, and two of the six probes exist specifically to
|
||
pin this location independence — see [One copy, one config](#one-copy-one-config).
|
||
|
||
### The blind spot: `references/` is unlinted, for two independent reasons
|
||
|
||
Every `references/*.md` file in the corpus is outside the prose gate. Count them with
|
||
`git ls-files | grep -cE '^plugins/[^/]+/\.apm/skills/[^/]+/references/.*\.md$'` rather than reading
|
||
a figure here; it moves with every retrofit. This is the gap that matters most, because the context
|
||
contract's own remedy for an over-long body is to move prose **into** `references/` — the gate pushes
|
||
text across its own boundary and then stops watching it. `factory-audit` is the live example. Its
|
||
dispatch body keeps only the gotchas common to both flows, and the flow-specific gotchas live under
|
||
`## Gotchas` in `references/skill-flow.md` and `references/agent-flow.md` (ADR-0025). Handing both
|
||
flow files to `vale-wrap.sh` prints `0 errors … in 0 files` and exits 0.
|
||
|
||
**Closing either cause alone changes nothing.** There are two, and they are independent:
|
||
|
||
| Cause | Where | Effect on a `references/` file |
|
||
|---|---|---|
|
||
| the `Kyberforge` style is scoped `[**/SKILL.md]` | `factory-audit/assets/vale/.vale.ini` | matches no section, so Vale lints 0 files and exits 0 |
|
||
| the hook's `files:` regex is `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$` | `vale-audit-prefilter-skill` in `.pre-commit-config.yaml` | the file is never handed to Vale at all |
|
||
|
||
Verified both ways. Handing `factory-audit`'s `vale-wrap.sh` a reference file directly — bypassing
|
||
pre-commit entirely, so only the style scope is in play — prints `0 errors … in 0 files` and exits 0,
|
||
where the same wrapper on a `SKILL.md` reports `in 1 file`. And the hook's `files:` regex, applied to
|
||
`git ls-files`, selects only the skill-directory `SKILL.md` files scoped at the top of this page, so
|
||
pre-commit never hands Vale a reference file to begin with. Widening the glob to `[**/*.md]` would
|
||
still lint nothing through the hook; widening the hook's `files:` alone would hand Vale files its own
|
||
config declines to match, which is the [0-file NOT RUN](#a-0-file-vale-run-is-not-run) shape — a
|
||
green run that measured nothing. **Issue #117** records the style-scope half; the hook half has to
|
||
land in the same change or the fix is cosmetic.
|
||
|
||
### `vale-wrap.sh`, never bare `vale`
|
||
|
||
`factory-audit`'s Step 1 and both pre-commit hooks call
|
||
`factory-audit/scripts/vale-wrap.sh`, not `vale`. It works around a confirmed **Vale 3.15.2** limitation:
|
||
`text.frontmatter.description` silently stops matching on most — not all — multi-line descriptions.
|
||
|
||
Verified by reproduction on a deliberately-bad fixture, not assumed:
|
||
|
||
| Description scalar spanning 2+ lines | Vale's behaviour |
|
||
|---|---|
|
||
| `>` folded block | 0 alerts, exit 0 — **broken** |
|
||
| plain (unquoted) continuation lines | 0 alerts, exit 0 — **broken** |
|
||
| single- or double-quoted, wrapped | 0 alerts, exit 0 — **broken** |
|
||
| `\|` literal block | alerts fire, exit 1 — lints normally |
|
||
|
||
The wrapper flattens the three broken forms to a single-line scalar in a scratch copy — or, for the
|
||
rare value no inline scalar can spell verbatim, a `|-` block with one content line — padding with
|
||
blank lines so **every other line number is unchanged**. `|` literal blocks and single-line
|
||
descriptions pass through untouched. Most descriptions in this repo are `>` blocks, so before the
|
||
wrapper a bad description in any of the three broken forms sailed straight through the prefilter.
|
||
|
||
### The `--config` argv defect
|
||
|
||
Handed **no `--config` at all**, the wrapper falls back to its own sibling `assets/vale/.vale.ini`,
|
||
located from `${BASH_SOURCE[0]}` rather than from the cwd. That is why both hooks' `entry:` is
|
||
the bare script path with **no argument after it**.
|
||
|
||
pre-commit prefixes only `entry[0]` with the hook-repo clone path (`cmd = (prefix.path(cmd[0]),
|
||
*cmd[1:])`), so every later argument resolves against the **consuming** repo's root. A `--config` in
|
||
the since-retired `.pre-commit-hooks.yaml` therefore pointed at a path no consumer has and
|
||
hard-failed every external run with `E100 [--config] Runtime error`.
|
||
|
||
`.pre-commit-config.yaml` drops the argument too, deliberately matching that entry.
|
||
The local `repo: local` hook resolved its `--config` correctly only because the consuming repo *was*
|
||
this repo — and that divergence is why three review rounds exercised a path no external consumer
|
||
takes and missed the defect. **Do not reintroduce a `--config` to either hook to make the local run
|
||
"explicit"**, and keep a restored published manifest to `entry[0]` alone (ADR-0014).
|
||
|
||
An explicit `--config` from any other caller still wins, in all three argv forms (`--config X`,
|
||
`--config=/abs`, `--config=rel`), and a relative one resolves against the caller's cwd — matching
|
||
bare `vale`, not the repo root.
|
||
|
||
`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 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
|
||
along with the two agent ones.
|
||
|
||
### A 0-file Vale run is NOT RUN
|
||
|
||
Vale reports 0 files only when the path it is handed matches **no glob section at all** — a
|
||
differently-named file, or a directory argument holding nothing that matches. That run prints
|
||
|
||
```
|
||
✔ 0 errors ... in 0 files.
|
||
```
|
||
|
||
and exits 0, indistinguishable from a clean pass. Both audits therefore treat a 0-file Vale run as
|
||
**NOT RUN** and fall back to full LLM judgment rather than reporting the Description dimension
|
||
clean.
|
||
|
||
### Pre-push
|
||
|
||
`vale` is still a **pre-push** dependency, but no longer through a hook of its own.
|
||
`check-vale-style-sync` — the hook that ran the six glob probes, and whose
|
||
`CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1` opt-out downgraded them audibly rather than skipping the
|
||
hook — is deleted with the second Vale copy (ADR-0025). The six glob probes survive it inside
|
||
`test-vale-wrap.sh`, so `run-tests --strict` is now the gate that runs them. That is also what keeps
|
||
`vale` a pre-push requirement: `test-vale-wrap.sh` exits 77 without the binary once its static cases
|
||
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, 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
|
||
|
||
House convention: banned phrasing that must be **mentioned** rather than used goes in backticks or a
|
||
fenced code block. Vale skips code spans and fences, so no suppression is needed — which is why this
|
||
document quotes `Use proactively` and "There is/are" the way it does.
|
||
|
||
Inline `<!-- vale Rule = NO -->` is the fallback **only** where backticking is impossible. Use the
|
||
HTML-comment form; the MDX `{/* */}` form does not work in plain Markdown. The one time a rule forced
|
||
suppression comments, the rule was deleted instead (see the `VagueQualifier` entry above).
|
||
|
||
## Tests
|
||
|
||
```
|
||
bash tests/run-tests.sh # every test-*.sh plus the bats suite
|
||
bash tests/run-tests.sh --bats-only # just bats
|
||
```
|
||
|
||
First run auto-initializes the bats submodules; no manual `git submodule update` needed.
|
||
|
||
**Exit 77 = SKIPPED.** A suite that skips because a dependency is missing does **not** fail an ad-hoc
|
||
run. The pre-push hook invokes the same script as `--strict` (`RUN_TESTS_STRICT=1` is equivalent),
|
||
where a skip **does** fail the push: at pre-push a skip means one of the documented dependencies is
|
||
absent on this machine, so the gate would otherwise report success having run fewer suites than it
|
||
appears to. Without `--strict` the gate once went green having verified 15 of 17 suites on a
|
||
vale-less PATH, with the skip list swallowed. Without vale, one suite skips — `test-vale-wrap.sh` —
|
||
and the strict failure names it and what to install. (It was three until
|
||
`test-check-vale-style-sync.sh` was deleted with its hook — see
|
||
[One copy, one config](#one-copy-one-config) — and `test-vale-hooks-consumer.sh` with the published
|
||
hook manifest.)
|
||
|
||
**Output assertions use a here-string, never a pipe.** Write `grep -q PATTERN <<< "$OUT"`, not
|
||
`echo "$OUT" | grep -q PATTERN`. Under `set -o pipefail` the pipe form fails depending on timing:
|
||
`grep -q` exits on its first match, `echo` takes SIGPIPE on its next write, and pipefail reports that
|
||
as the pipeline failing, so output that matched reads as "no match". It showed up as a push gate that
|
||
failed about once in 670 runs, on a different suite each time.
|
||
|
||
`tests/test-no-pipefail-early-exit-grep.sh` is the static guard. **What it scans** is the tracked
|
||
`*.sh`, `*.bats` and `*.bash` files, minus itself — `git ls-files -- '*.sh' '*.bats' '*.bash'` — and,
|
||
within each one that sets pipefail, it fails on the pipe form. Every tracked shell file in this tree
|
||
carries one of those three extensions today, so "every tracked shell file" is true in effect, but it
|
||
is a property of the tree and not of the scan: a shell script tracked under any other name is not
|
||
reached. It checks its own scanner against fixtures before trusting a clean result.
|
||
|
||
**What the pattern models**, after a review widened it on four axes and narrowed it on one:
|
||
|
||
- **The reader** is `grep`, `egrep` or `fgrep` — behind a path prefix (`/bin/grep`), a `command`
|
||
prefix, or env-var assignments (`LC_ALL=C grep`) — taking `-q`, `-m`, `-l`, `-L`, `--quiet`,
|
||
`--silent`, `--max-count` or `--files-with`. (The last two match the regex and were missing from
|
||
this list.)
|
||
- **Intermediate stages are seen.** `echo x | filter | grep -q y` is a site; only the two-stage form
|
||
used to be.
|
||
- **Both line continuations are joined** before matching: a trailing backslash, and a trailing `|`,
|
||
which is equally legal in a pipeline. The hit is reported at the line the command starts on.
|
||
- **`pipefail` may sit anywhere in a `set` line**, so `set -o errexit -o pipefail` arms the file. It
|
||
previously had to follow the *first* `-o`, and a file-level miss skips every site in that file
|
||
rather than one — which is why that test is deliberately loose.
|
||
- **A bare `&` ends a segment**, so `echo ok && other | grep -q x` — whose writer is `other`, not the
|
||
`echo` — is not a site. `&` followed by a digit is kept, so `2>&1` does not end one.
|
||
|
||
The widening turned up **five live vulnerable sites in `tests/test-apm-current-hook.sh`** that the
|
||
narrower scanner never saw while the suite reported green; all five are converted, and the scanner
|
||
now reports zero over the tree. Its fixtures went from 4 vulnerable spellings to **12**, plus
|
||
near-miss negatives it must leave alone, and the suite from 5 cases to **7**.
|
||
|
||
**Remit: early-exiting GREP readers only.** `head`, `sed -n 1p` and a bare `read` exit early too, and
|
||
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. 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
|
||
vulnerable-looking line would be reported as a real site. There are none in the tree today.
|
||
|
||
`tests/run-bats.sh` derives the set of `.bats` files it expects from `git ls-files`, so a `.bats`
|
||
file deleted from the worktree but still tracked in the index fails the run rather than silently
|
||
shrinking the suite. Remove one with `git rm` (or stage the deletion) when intentional; an untracked
|
||
new `.bats` file is picked up and needs no ceremony.
|
||
|
||
Both discovery walks (`tests/run-bats.sh` and `tests/run-tests.sh`) exclude `apm_modules/`:
|
||
`apm install` materializes a full copy of every plugin there, and running a dependency's copy of a
|
||
`.bats` file breaks its relative path to the bats helpers — **167 spurious failures** before the
|
||
exclusion landed.
|
||
|
||
## apm gates
|
||
|
||
### `apm-audit-ci`
|
||
|
||
Runs `apm audit --ci` **once per manifest** — the root one and each of the six plugin packages —
|
||
because the root-only invocation audits the marketplace manifest and **nothing else**, and
|
||
`apm-pack-check-clean` does not parse plugin `dependencies:` blocks either. Verified: a malformed
|
||
dependency entry passes `apm pack --check-versions --check-clean --dry-run` and fails
|
||
`apm audit --ci` in that package's directory. Costs ~0.5s per package.
|
||
|
||
**What it actually runs is asymmetric**, and the two manifest classes are not comparable. Verified by
|
||
running `apm audit --ci` (apm 0.28.0) at the repo root and in `plugins/lint/`, reading the check
|
||
names straight off its own compliance table:
|
||
|
||
On the **root** manifest, **10 checks**: `lockfile-exists`, `ref-consistency`,
|
||
`deployment-ledger-owners`, `deployed-files-present`, `no-orphaned-packages`,
|
||
`skill-subset-consistency`, `config-consistency`, `content-integrity`, `includes-consent`, `drift`.
|
||
|
||
On each **plugin** manifest, **1 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 in the list above is root-only, because it is
|
||
the root install that has a lockfile, a deployment ledger and deployed files to check.
|
||
|
||
**`manifest-parse` is not a named check** in apm 0.28.0's output, and an earlier revision of this
|
||
section listed 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 rather than appearing as a row in it.
|
||
|
||
**The hook needs a completed `apm install`.** `deployed-files-present` checks the install output on
|
||
disk, so on a fresh clone it fails with `303 deployed file(s) missing` and takes the push with it.
|
||
That is not a defect in the gate; it is the gate correctly reporting that nothing has been installed
|
||
yet. Run `apm install` before the first push from a new checkout.
|
||
|
||
It does **not** enforce an org policy. apm discovers one from the git remote and only understands
|
||
github.com and Azure DevOps, so against this repo's self-hosted Gitea remote it prints:
|
||
|
||
```
|
||
No org policy found at unknown; enforcement skipped
|
||
```
|
||
|
||
**Do not "fix" that with `policy.fetch_failure_default: block` in `apm.yml`.** apm's own message
|
||
suggests it; it was tried on a scratch copy and **rejected**. With no reachable policy source it does
|
||
not make the check meaningful, it makes it permanently red — `apm audit --ci` exits 1 with
|
||
`No org policy found at unknown (policy.fetch_failure_default=block)` on every push, forever. A gate
|
||
that can never go green is not a gate. Revisit only if this repo gains a policy source apm can reach.
|
||
|
||
**It does scan for hidden Unicode.** An earlier revision of this section said the opposite. The
|
||
`content-integrity` check in the root table *is* that scan — it reports `No critical hidden Unicode
|
||
or hash drift detected` — so the root invocation already covers it and nothing needs adding. What
|
||
remains true is that the *standalone* mode is different: 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.
|
||
Plugin manifests get `lockfile-exists` and nothing else; they are not Unicode-scanned.
|
||
|
||
### `check-executables-allow-sync`
|
||
|
||
apm gates a package's `hooks/` and `bin/` on root `apm.yml`'s `executables.allow`
|
||
(`apm_cli/security/executables.py`). `is_package_approved` is itself an exact dictionary lookup, but
|
||
it is never called with a single key: `install/exec_gate.py` builds a candidate list that includes
|
||
the version-blind name alongside `<package>#<version>`, and `materialize_exec_map` stores every
|
||
approved key **under its version-blind name as well**. `_map_grants` matches the same three ways.
|
||
|
||
**Correction (2026-09-19):** verified against apm 0.28.0, a kyberforge version bump therefore does
|
||
*not* stop the entry matching — approving `owner/repo#2.0.0` also covers `owner/repo#2.1.0` through
|
||
the version-blind alias. The earlier claim here ("no wildcard and no version-less form", so the
|
||
entry silently stops matching and the `SessionStart` hook stops deploying) described apm's behaviour
|
||
wrongly, and ADR-0019 carries the same correction.
|
||
|
||
The gate is still required, for a repo-level reason rather than an apm-level one:
|
||
`scripts/check-executables-allow-sync.sh` asserts the key matches `plugins/kyberforge/apm.yml`'s
|
||
`version:`, so a bump without a key edit fails *this repo's* pre-push, and the key stays an accurate
|
||
record of what was approved.
|
||
|
||
`scripts/check-executables-allow-sync.sh` parses `version:` out of `plugins/kyberforge/apm.yml` and
|
||
asserts root `apm.yml` carries the matching `kyberforge#<version>` key. A comment in the
|
||
`executables:` block 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.
|
||
|
||
## `.claude/settings.json`
|
||
|
||
**apm owns this file. Nothing repo-authored goes in it.**
|
||
|
||
`apm audit --ci` replays the install into a scratch tree and diffs the result byte-for-byte, so
|
||
anything apm would not have written there — an `enabledPlugins` block, a real `hooks` entry — is
|
||
permanent drift that fails `apm-audit-ci`. A hook you want in this repo is authored in
|
||
`plugins/<name>/.apm/hooks/` and deployed by apm, never hand-written here.
|
||
|
||
Its committed content is whatever apm last wrote, which today is the merged `SessionStart` entry for
|
||
kyberforge's `check-apm-current.sh`. That is apm's own output and it belongs in the commit (ADR-0019;
|
||
ADR-0018's statement that the committed content is exactly `{"hooks": {}}` is superseded on that
|
||
point only). Machine-specific settings go in the gitignored `.claude/settings.local.json`, which apm
|
||
does not deploy and the replay does not compare; shared enforcement belongs in
|
||
`.pre-commit-config.yaml`.
|
||
|
||
### Why it is excluded from `pretty-format-json`
|
||
|
||
It is in the **second and last alternation** in that hook's `exclude:` pattern, and that alternation
|
||
is the only one there for a reason other than "generated manifest". Mind which number you are
|
||
quoting: the pattern is `^(\.claude-plugin/marketplace\.json|\.claude/(settings|apm-hooks)\.json)$`
|
||
— **two top-level alternations, expanding to three real tracked files**:
|
||
`.claude-plugin/marketplace.json`, this one, and its committed `.claude/apm-hooks.json` sidecar,
|
||
which is apm output under the same byte-for-byte replay and is excluded for the same reason.
|
||
|
||
`pretty-format-json --autofix` sorts object keys unless `--no-sort-keys` is passed, while apm's hook
|
||
integrator emits insertion order (`matcher` before `hooks`, `type` before `command`). Leaving the
|
||
file in that hook's scope therefore rewrites apm's output into a form apm would never produce on the
|
||
way into **every** commit, and `apm-audit-ci` then reports permanent drift on a file with an empty
|
||
`git diff` — exactly what happened when the `SessionStart` hook first landed in `2e395a4`. Re-running
|
||
`apm install` fixes the file; leaving it in scope would re-break it on the very commit carrying the
|
||
fix.
|
||
|
||
**Load-bearing. Do not tidy it out of that list** (see `LESSONS.md`, 2026-08-14).
|
||
|
||
## Pushing without a network
|
||
|
||
No pre-push hook needs the network **once `apm install` has populated `apm_modules/`**. Every entry
|
||
in root `apm.yml`'s `marketplace.packages[]` resolves from a local `./plugins/<name>` path, so
|
||
`apm-pack-check-clean` never calls `git ls-remote`.
|
||
|
||
`apm-audit-ci` calls `apm` too, and its org-policy discovery resolves nothing on this remote before
|
||
any network call. But it is local only against a populated install: `drift` and `config-consistency`
|
||
replay the install to diff scratch against the working tree, and that replay is cache-only —
|
||
`[>] Replaying install (cache-only)` — which is exactly why it costs no network here. On a **fresh
|
||
clone** there is no cache to replay from, so the replay clones from the holocron remote and those two
|
||
checks fail offline, with `deployed-files-present` already failing for the same reason (see
|
||
`apm-audit-ci` above). 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.
|
||
|
||
---
|
||
|
||
## See also
|
||
|
||
- `docs/adr/0020-skill-description-and-body-context-contract.md` — the context contract, its
|
||
enforcement table (deterministic vs. auditor judgment), and every rejected alternative
|
||
- `docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md` — the `SessionStart` hook, the
|
||
executable-trust gate, and the version-pinned allow key
|
||
- `docs/adr/0024-apm-is-the-only-supported-install-path.md` — apm as the sole install path, and the
|
||
deletion of the flat content mirror and its `check-plugin-content-sync` gate. It supersedes
|
||
`docs/adr/0017-plugin-content-mirror-bridges-apm-to-host-discovery.md` (**superseded** — plugin
|
||
content sync, kept as the historical record)
|
||
- `docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md`,
|
||
`docs/adr/0014-vale-prefilter-ships-from-the-plugin.md` — apm-generated manifests, committed Vale
|
||
styles
|
||
- `docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md` — the audit-pair merge that
|
||
collapsed the two Vale copies to one, removed the `check-vale-style-sync` hook, and took the shared
|
||
boundary resolver from three copies to two (one since `ef27c97`). It amends ADR-0014 and ADR-0020
|
||
on those points
|
||
- `docs/spec/architecture.md` — directory structure, install pipeline, what is generated and what is
|
||
hand-authored
|
||
- `.pre-commit-config.yaml` — the hooks themselves, with inline rationale comments
|