The #113 sweep rested on CLAUDE.md's premise that rtk either filters or passes through unchanged, so prefixing is always safe. Measured against rtk 0.42.4, that premise is false for several of the commands the sweep prefixed, and two skills were left giving wrong answers silently. Why: - `rtk git worktree list --porcelain -z` discards both flags and renders its own format. The `locked`/`lock_reason` fields git-worktrees Step 2 must emit are absent entirely, and paths under $HOME are abbreviated to `~/`. - `rtk git branch --list <name>` prints a phantom `* ` line even when nothing matches, so git-branches' stated ambiguity test — "output from both means the name is ambiguous" — reported every name as ambiguous. `tag --list` is a clean passthrough, so only one half broke. - `rtk git diff --name-only`/`--name-status` append a `Changes:` trailer to output documented as "one per line"; `--word-diff` emits none of the `[-removed-] {+added+}` markers its table describes; `rtk git log -L` truncates each line at ~72 chars, on the one command whose purpose is showing line content. - `rtk git stash pop` prints only `FAILED: git stash pop`, swallowing the conflict diagnostic and retained-entry message the surrounding prose tells the agent to rely on. Implementation notes: - Eleven sites reverted to bare `git`, each carrying its reason inline so the next sweep does not undo it. `mergetool` and `rebase -i` are reverted on clause 3's interactive limb only: the TTY defect does not reproduce — rtk filters exactly twelve subcommands and execs the rest — and ADR-0023 records that measurement rather than a convenient one. - ADR-0023 states the rule repo-wide with a third clause: a command whose output the skill parses, or which is interactive, stays bare. `plugins/git/README.md` is reduced to a pointer; its claim that gitea skills "contain no git/rtk mentions at all" was false, and its citation of `hard-rules.md` pointed at a file containing no occurrence of "rtk". - Eight gitea sites swept, all verified byte-identical passthroughs first. - `scripts/check-rtk-prefix.sh` gates clause 1. Run against main's pre-sweep corpus it reports 99 findings including every gitea site, so it would have caught the drift #113 was filed about. Impact: the gate covers clause 1 only, in shell-tagged fences and the opening span of Run cells. Clause 2 is not gateable — "Run `git switch`" and "`git switch` refuses" are the same tokens — and prose bullets are invisible to it. Both limits are recorded in gates.md rather than left implied. Refs: #113 ADR: 0023 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeH8SCbcrCAQrtymkNuhKP
1049 lines
64 KiB
Markdown
1049 lines
64 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 — with one caveat below |
|
||
| `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 **16** hooks, not 14. 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`. Fourteen
|
||
is the count of hooks this repo authors itself.
|
||
|
||
**The caveat: one of those 14 is a silent no-op under that invocation.**
|
||
`check-release-needed` exits 0 immediately unless `PRE_COMMIT_REMOTE_BRANCH` equals
|
||
`refs/heads/main`, and pre-commit exports that variable only from the real pre-push git hook during
|
||
an actual `git push`. Running the stage by hand — or from a CI runner — therefore reports it
|
||
`Passed` having checked nothing. That is by design for feature branches — pushing WIP must not be
|
||
blocked on cutting a premature tag — but it means `--hook-stage pre-push --all-files` is a full
|
||
rehearsal of 13 hooks and a skip of the fourteenth. The script's own header records the same gap for
|
||
a PR merged through Gitea's merge button, where no local push happens at all.
|
||
|
||
## The pre-push gate
|
||
|
||
Fourteen 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)) |
|
||
| `check-manifests` | `marketplace.json` and `plugin.json` paths resolve (needs `jq`) |
|
||
|
||
**Generated-content drift gates**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `check-plugin-content-sync` | each plugin's flat `skills/agents/commands/hooks` mirror matches `.apm/` (issue #90) |
|
||
| `check-marketplace-mirror-sync` | `.github/plugin/marketplace.json` is byte-identical to `.claude-plugin/marketplace.json` — no apm output profile targets that path |
|
||
| `check-vale-style-sync` | skill-audit's Vale copy matches agent-audit's canonical copy, plus six glob-coverage probes (see [Vale](#vale)) |
|
||
| `check-scope-walkup-sync` | `validate.sh`, `validate-provenance.sh`, `new-agent.sh` and `new-skill.sh`'s four independent `$HOME`/`.git`/`apm.yml` walk-up ports still agree behaviorally |
|
||
| `check-executables-allow-sync` | root `apm.yml`'s `executables.allow` key names kyberforge's actual version (see [apm gates](#apm-gates)) |
|
||
|
||
`check-executables-allow-sync` is the odd one in this group: it guards a *silent failure* rather than
|
||
drift in generated text.
|
||
|
||
**Artifact validators**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `check-apm-agents-valid` | runs agent-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-marketplace-check` | every `marketplace.packages[]` entry resolves, including network reachability of remote refs |
|
||
| `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** (both need the `claude` CLI on PATH)
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `validate-plugins` | `claude plugin validate --strict` on every plugin directory |
|
||
| `validate-marketplace` | `claude plugin validate --strict` on the root marketplace manifest |
|
||
|
||
**Release**
|
||
|
||
| Hook | Guards |
|
||
|---|---|
|
||
| `check-release-needed` | on a real `git push` to `main` only — fails if files exposed via `.pre-commit-hooks.yaml` changed since the last tag. A no-op everywhere else, including under `pre-commit run --hook-stage pre-push` (see [the caveat above](#running-the-gates)) |
|
||
|
||
Four of these shell out to `apm`: `apm-marketplace-check`, `apm-audit-ci`, `apm-pack-check-clean`,
|
||
and `check-plugin-content-sync` (via `scripts/sync-plugin-content.sh`, which wraps `apm pack`). The
|
||
first and third are bare `apm …` entries and the second 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`. `jq` is
|
||
needed by `scripts/check-manifests.sh` and `scripts/sync-plugin-content.sh` — those at least fail
|
||
loudly (`Error: jq is required but not installed`).
|
||
|
||
## 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`. It is also shipped to external repos as
|
||
`kyberforge-skill-size-check` (see
|
||
[External consumers](#external-consumers-the-root-pre-commit-hooksyaml)).
|
||
|
||
**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. The remaining unmatched `SKILL.md` files in the tree are the
|
||
generated flat mirror, which is excluded by the `.apm/` segment on purpose — a mirror edit is drift,
|
||
not an authoring change.
|
||
|
||
### `skill-frontmatter`, the other hook on that scope
|
||
|
||
A second `repo: local` pre-commit hook, `skill-frontmatter`, runs on the **same** `files:` pattern at
|
||
the same stage. It is a shell loop that, **for the YAML frontmatter block only** — everything between
|
||
the opening `---` and the next `---` — asserts four things per file:
|
||
|
||
| Check | Rejects with |
|
||
|---|---|
|
||
| a `^name:` line is present | "missing required frontmatter fields (name: …)" |
|
||
| a `^description:` line is present | "missing required frontmatter fields (description: …)" |
|
||
| `metadata:` contains a `^ version:` key, anchored, scanning to the next top-level key | "missing required frontmatter fields (metadata.version)" |
|
||
| that version's value is three-part semver (`1.0.0`, quoted or not) | "has a malformed frontmatter metadata.version (…)" |
|
||
|
||
Every one of those qualifiers is load-bearing, and each replaced a defect that let the hook report
|
||
Passed having measured nothing. `tests/test-skill-frontmatter.sh` pins all of them:
|
||
|
||
- **Frontmatter-scoped, not whole-file.** The checks used to `grep` the entire file, so a `metadata:`
|
||
or `name:` block quoted in a **body code fence** satisfied them — `skill-author`'s own docs quote
|
||
exactly such a block.
|
||
- **Bounded by the next top-level key, not by `-A10`.** The version check was
|
||
`grep -A10 "^metadata:" | grep -q " version:"`, which ran ten lines past the end of the block: a
|
||
`version:` belonging to a following `source:` list entry counted (`write-docs` and `research` both
|
||
have a `source:` list immediately after `metadata:`), while a `metadata:` block with more than ten
|
||
lines before its `version:` was reported missing.
|
||
- **`^ version:` anchored.** `" version:"` was an unanchored substring, so a deeper-nested
|
||
` version:` matched too.
|
||
- **The value is asserted, not just the key.** `plugins/bin/.apm/skills/write-docs/SKILL.md` carried
|
||
`version: "1.0"` — present, correctly nested, and not a version — through an entire PR under a
|
||
presence-only check. Two-part `1.0` is a YAML float, not a version string.
|
||
- **The call shape is pinned.** `entry: bash` with `args: ['-c', <script>, …]` needs an explicit
|
||
arg0 placeholder after the script: without it `bash -c` puts pre-commit's **first** filename in
|
||
`$0`, where `for f in "$@"` never sees it. A single-file commit — the normal case — therefore ran
|
||
the loop body zero times and exited 0. The third `args` entry (`skill-frontmatter`) exists solely
|
||
to absorb `$0`; do not remove it.
|
||
- **An unreadable file is an error, not a pass.** A file with no closing `---` fails with "no closing
|
||
YAML frontmatter block" rather than falling through to a green.
|
||
|
||
**It still overlaps ADR-0020's "description present and non-empty" FAIL, and the overlap is not
|
||
clean.** The ADR (`:95-101`) requires that question be decided on the **YAML-folded value** and
|
||
nowhere else, precisely because a line regex gets it wrong in both directions. Measured on fixtures:
|
||
|
||
| Frontmatter | `skill-frontmatter` | `skill-size-check` |
|
||
|---|---|---|
|
||
| `description:` with no value, then `model: sonnet` | passes — the key is on a line | ERROR, "missing or empty" |
|
||
| `"description": …` (quoted key, valid YAML) | **fails** — `^description:` does not match | passes, description read normally |
|
||
|
||
So the grep is not a second opinion on presence. It is blind to the shape ADR-0020 was written
|
||
against, and it is the only one of the two that objects to a quoted key. Neither disagreement is
|
||
currently live in the corpus, and the honest reading is that presence is `skill-size-check`'s
|
||
question — the grep's contribution to it is noise on one shape and silence on the other.
|
||
|
||
What the hook adds that **no** ADR-0020 check reads is two keys: `name:` and `metadata.version`. A
|
||
`SKILL.md` missing either passes `skill-size-check` at exit 0. That is its unique coverage, and the
|
||
reason not to fold it into the size gate on the grounds of redundancy.
|
||
|
||
#### Why this one stays a shell parser
|
||
|
||
[`python3` and PyYAML are hard requirements](#python3-and-pyyaml-are-hard-requirements) below records
|
||
that a hand-rolled frontmatter reader on this exact `files:` scope was **deliberately deleted**,
|
||
because "a reader that mis-parses an unfamiliar scalar shape reports a clean pass on a file it never
|
||
measured." That reasoning is about `skill-size-check` and does **not** transfer here. Do not delete
|
||
this hook citing it. Three differences:
|
||
|
||
1. **It answers a strictly narrower question.** `skill-size-check` must know the *folded value* of a
|
||
`>`-block scalar to count its characters, which is where a line reader diverges from a parser —
|
||
one corpus description measured 270 characters parsed and 412 unparsed. This hook asks only
|
||
whether a key is on a line and whether one short **plain scalar** matches `N.N.N`. There is no
|
||
folding, no multi-line value, and no measurement to get subtly wrong.
|
||
2. **It is frontmatter-scoped.** The failure mode that killed the old fallback was silently reading
|
||
past or short of the block. This one extracts the block explicitly and errors out when it cannot
|
||
find a closing marker, so "could not parse" is a red, never a green.
|
||
3. **It is pinned by tests.** `tests/test-skill-frontmatter.sh` drives the hook through pre-commit's
|
||
real `bash -c <script> <arg0> <files…>` invocation and asserts each defect class above. The
|
||
deleted fallback had no such suite; that is how its disagreement with a real parser survived.
|
||
|
||
The trade it buys is that the hook stays repo-local. Moving it to a script would change the
|
||
externally exposed `.pre-commit-hooks.yaml` contract for consumers, for a check that has no need of a
|
||
YAML parser.
|
||
|
||
### 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 39-skill universe into every consumer repo running the
|
||
hook through pre-commit, so a consumer skill routing to `skill-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 fixed: running the hook over a tree holding only `plugins/` and the root `apm.yml`, with no
|
||
`.claude/` or `.agents/` anywhere, produced findings identical to the working tree. The figures that
|
||
reproduction recorded — 26 description FAILs, 9 body FAILs, 2 dangling targets, 0 missing references
|
||
— are the pre-retrofit corpus as it stood when the experiment was run, kept here as the evidence for
|
||
the install-independence claim. They are not current: the retrofit under #99 took the first three to
|
||
zero. What the experiment establishes is that the two trees agree, not what either measured.
|
||
|
||
### 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
|
||
|
||
`skill-audit`'s `validate.sh` holds a second copy of the four ADR-0020 constants
|
||
(`DESC_SUGGEST_CHARS` / `DESC_MAX_CHARS` / `BODY_SUGGEST_WORDS` / `BODY_MAX_WORDS`), and
|
||
`agent-audit`'s `validate.sh` holds a third copy of the two description constants. They are copied
|
||
rather than imported because a cache-installed plugin's scripts cannot read files outside their own
|
||
plugin directory. `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 block is embedded verbatim in all three scripts between `BEGIN`/`END ADR-0020 SHARED
|
||
BOUNDARY RESOLVER` markers and must stay byte-identical.
|
||
|
||
### `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. 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 on this scope, and one deliberate exception sits
|
||
right next to it.** [`skill-frontmatter`](#skill-frontmatter-the-other-hook-on-that-scope) runs on the
|
||
same `files:` pattern as a **shell** parser, on purpose — it asks only whether a key is on a line and
|
||
whether one short plain scalar matches `N.N.N`, with no folding to get wrong, and moving it to a
|
||
script would change the externally exposed `.pre-commit-hooks.yaml` contract for consumers. That
|
||
section carries the full argument. A reader arriving here first should not read this one as
|
||
condemning it. `check-rtk-prefix` needs `python3` but **not** PyYAML: it reads the markdown body and
|
||
never touches frontmatter, so it has no scalar to fold.
|
||
|
||
## Agent files take the description gates, not the body gate
|
||
|
||
`check-apm-agents-valid` runs agent-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 (agent-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
|
||
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 in agent-audit's validator — adding a body gate there contradicts the ADR rather than fixing
|
||
an inconsistency.
|
||
|
||
**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.
|
||
|
||
## Current retrofit status
|
||
|
||
**The ADR-0020 gates ship hot, with no baseline file.** A shrinking baseline recording each
|
||
non-compliant skill's current numbers was considered and rejected in favour of hot gates.
|
||
|
||
**The corpus is now clean on both gates.** Issue **#99** retrofitted all 39 skills plugin by plugin;
|
||
`kyberforge` was the last wave, after which the corpus was swept as a whole rather than per plugin.
|
||
Each sweep is followed by an **independent review round**: a fresh agent with no memory of the
|
||
retrofit re-measures the corpus and files what it finds, and the round repeats until one lands no
|
||
findings. The rounds are recorded as comments on **#99** — read the current state off that thread,
|
||
which is why no round count is pinned here.
|
||
|
||
| Gate | Current findings |
|
||
|---|---|
|
||
| `skill-size-check` | **0 of 39** descriptions and **0 of 39** bodies exceed their FAIL tier; 0 dangling targets; SUGGESTIONs outstanding (count not pinned — see below) |
|
||
| `Kyberforge.CompositionNote` (Vale) | **0 errors** — the four `gitea-*` carriers were all retrofitted |
|
||
|
||
**The SUGGESTION count is deliberately not recorded here.** It moves with every skill edit *and*
|
||
with every change to the gate's own tiering, so any figure written down is stale by the next commit.
|
||
Measure it instead:
|
||
|
||
```
|
||
bash scripts/skill-size-check.sh plugins/*/.apm/skills/*/SKILL.md | grep -c '^SUGGESTION'
|
||
pre-commit run skill-size-check --all-files # same findings, via the hook
|
||
```
|
||
|
||
A non-zero count is the expected steady state, not a regression. SUGGESTIONs exit 0 and block
|
||
nothing; only the two FAIL tiers, the dangling-target ERROR and the missing-`references/` ERROR do.
|
||
Read the count as a work queue, and the FAIL columns above as the gate.
|
||
|
||
`Kyberforge.CompositionNote` is the ADR-0020 Vale rule banning composition and architecture prose
|
||
from a description. Every Vale rule here is `level: error` with no ignorable tier, so a description
|
||
that reintroduces one blocks the commit even though no skill carries one today.
|
||
|
||
Because nothing is grandfathered, the gates now bite on **first commit**: a new skill, or an edit
|
||
that pushes a description past 400 characters or a body past 900 words, is blocked until it
|
||
complies. That is the steady state the retrofit was for — it is no longer true that an unrelated
|
||
one-line fix to a skill requires retrofitting that skill first.
|
||
|
||
Check where a skill stands before starting, and check **both** gates:
|
||
|
||
```
|
||
pre-commit run skill-size-check --all-files # size/context only
|
||
pre-commit run --all-files # size AND Vale
|
||
```
|
||
|
||
Scoping a retrofit off `skill-size-check` output alone leaves you blocked at the second gate.
|
||
|
||
## 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.** A skill-directory README is consumer-facing prose no agent
|
||
loads, and the `git clone https://github.com/bats-core/…` lines in the seven `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/{skill-audit,agent-audit}/assets/vale/styles/`, not downloaded
|
||
packages (ADR-0014).
|
||
|
||
### Two copies, one canonical
|
||
|
||
Wiring Vale as a deterministic prefilter for `skill-audit`/`agent-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 **twice**,
|
||
one copy per skill, both under `plugins/kyberforge/.apm/skills/`:
|
||
|
||
| Copy | Styles | `.vale.ini` sections |
|
||
|---|---|---|
|
||
| `agent-audit/assets/vale/` — **canonical** | `Kyberforge`, `KyberforgeCopilot` | `[**/agents/*.md]`, `[**/*.agent.md]` |
|
||
| `skill-audit/assets/vale/` — smaller duplicate | `Kyberforge` | `[**/SKILL.md]` |
|
||
|
||
Duplicated rather than shared because a plugin's cache-install copies only each skill's own files —
|
||
there is no cross-skill sharing to point at. `check-vale-style-sync` at pre-push is what keeps them
|
||
from drifting; `KyberforgeCopilot` is the one deliberate inequality, being scoped only to `.agent.md`
|
||
files for the Copilot-only "`Use proactively` has no effect" check.
|
||
|
||
### What Vale owns, and what stays LLM judgment
|
||
|
||
Eleven rule files across the two copies, 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.
|
||
|
||
### External consumers: the root `.pre-commit-hooks.yaml`
|
||
|
||
The root `.pre-commit-hooks.yaml` exposes both Vale copies (`kyberforge-vale-audit-skill`,
|
||
`kyberforge-vale-audit-agent`) plus `kyberforge-skill-size-check`, so any external repo can enforce
|
||
the same rules with `repo: <this-repo-url>, rev: <tag>` in its own `.pre-commit-config.yaml`.
|
||
pre-commit clones the pinned rev into its own cache, independent of whether Claude Code or the
|
||
`kyberforge` plugin is installed at all; the same mechanism covers CI via `pre-commit run
|
||
--all-files`. `skill-size-check` has no external asset dependency, so it needed no relocation under
|
||
ADR-0014 — only exposure.
|
||
|
||
This repo's own `vale-audit-prefilter-skill` / `-agent` hooks consume the **identical**
|
||
plugin-bundled copies via `repo: local`. Deliberately not a third root copy, and deliberately **not a
|
||
pinned self-reference** — a pinned self-reference would lint working-tree edits against the last
|
||
tagged release rather than against the change being made.
|
||
|
||
### 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. A `SKILL.md` in the generated flat mirror matches
|
||
neither pattern, so prose findings surface only when you edit the file you are supposed to be
|
||
editing. Without the binary the hooks fail with a bare "command not found" and no install pointer.
|
||
|
||
**Two hooks, not one combined hook.** Both manifests split the prefilter in two precisely because a
|
||
single hook can point at only one copy, and that copy 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)).
|
||
|
||
### The `.vale.ini` globs do no scoping
|
||
|
||
Each `.vale.ini`'s section globs are **path-agnostic** — `[**/SKILL.md]` for skill-audit's copy,
|
||
`[**/agents/*.md]` and `[**/*.agent.md]` for agent-audit's — 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 hook's own `files:` regex and from the audit skills
|
||
passing one explicit file per invocation. The two manifests scope **differently on purpose**:
|
||
|
||
| Manifest | `-skill` | `-agent` |
|
||
|---|---|---|
|
||
| `.pre-commit-config.yaml` (pins this repo's layout) | `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$` | `^plugins/[^/]+/\.apm/agents/[^/]+\.agent\.md$` |
|
||
| `.pre-commit-hooks.yaml` (layout-agnostic for consumers) | `(^\|/)SKILL\.md$` | `(^\|/)agents/[^/]+\.md$\|\.agent\.md$` |
|
||
|
||
Narrowing a `.vale.ini` glob to a `plugins/`-shaped path to "tighten" it breaks the consumer case,
|
||
and `check-vale-style-sync`'s probe set is built to catch exactly that.
|
||
|
||
### 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.
|
||
|
||
**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]` | `skill-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 skill-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.
|
||
|
||
The consumer manifest is a third axis and does not rescue this either: `.pre-commit-hooks.yaml`'s
|
||
`(^|/)SKILL\.md$` is layout-agnostic but still filename-shaped, so an external repo running
|
||
`kyberforge-vale-audit-skill` has the same gap.
|
||
|
||
### `vale-wrap.sh`, never bare `vale`
|
||
|
||
Both audit skills' Step 1 and both pre-commit hooks call **each copy's own**
|
||
`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 manifests' `entry:` is
|
||
now 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
|
||
`.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 keeping the two entries identical.
|
||
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 manifest to make the local
|
||
run "explicit".**
|
||
|
||
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.
|
||
|
||
Both audit skills' 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.
|
||
|
||
`tests/test-vale-wrap.sh` regression-tests this against **skill-audit's** copy specifically: its
|
||
fixtures are all `SKILL.md`-shaped, and only skill-audit's `.vale.ini` carries that glob section.
|
||
|
||
### 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 a **pre-push** dependency too, not only pre-commit. `check-vale-style-sync` runs **six
|
||
glob-coverage probes** by invoking `vale --config` — one representative path per file shape the
|
||
prefilter is supposed to cover. They are the only assertions in the script that catch a `.vale.ini`
|
||
glob typo (`[**/SKILL.md]` → `[**/SKILLS.md]`), the failure mode where every text-level check stays
|
||
clean while vale lints zero files. As a warning this self-disabled on exactly that mutation and
|
||
exited 0, and since pre-commit swallows a passing hook's output the stderr line was never seen — the
|
||
hook reported `Passed`. Missing `vale` is therefore a hard failure here.
|
||
|
||
The opt-out is `CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1`, and **it is not `SKIP=`**: the hook
|
||
still runs and still asserts everything verifiable from file text, but the six probes do not, and its
|
||
summary says so explicitly —
|
||
|
||
```
|
||
Vale style sync check passed (text-level only, vale unavailable): … 0 glob probe(s) verified.
|
||
```
|
||
|
||
Use it only on a machine that genuinely cannot install `vale`, and read that line as "the glob axis
|
||
was not checked", not as a pass. The hook is `verbose: true` for exactly that reason — its clean
|
||
output is a single line, so it costs one line per push.
|
||
|
||
### 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, three suites skip —
|
||
`test-check-vale-style-sync.sh`, `test-vale-hooks-consumer.sh`, `test-vale-wrap.sh` — and the strict
|
||
failure names each one and what to install.
|
||
|
||
`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.
|
||
|
||
It verifies **exactly two things** per manifest and claims no more:
|
||
|
||
- **manifest-parse** — each `apm.yml` parses as a valid APM manifest. Unconditional; verified to fire
|
||
on a dependency entry missing its `git`/`path`/`registry` field (`Cannot parse apm.yml`).
|
||
- **lockfile-exists** — any package declaring dependencies has a consistent `apm.lock.yaml`.
|
||
Conditional, and vacuous while every plugin `apm.yml` declares `dependencies: {apm: [], mcp: []}`;
|
||
it arms itself the moment one does not (verified by adding a git dependency to
|
||
`plugins/lint/apm.yml`).
|
||
|
||
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 also does not scan for hidden Unicode: that scan is plain `apm audit`, a different mode (`--ci`
|
||
refuses to combine with `--file`/`--strip`/`--dry-run`/`PACKAGE`), and plain `apm audit` here reports
|
||
`No apm.lock.yaml found -- nothing to scan` and exits 0. Adding it would buy a second vacuous check.
|
||
|
||
### `check-executables-allow-sync`
|
||
|
||
apm gates a package's `hooks/` and `bin/` on an **exact `<package>#<version>` dictionary lookup** in
|
||
root `apm.yml`'s `executables.allow` (`apm_cli/security/executables.py`, `is_package_approved`).
|
||
There is no wildcard and no version-less form.
|
||
|
||
So bumping `plugins/kyberforge/apm.yml`'s `version:` without bumping the key **errors nowhere**: the
|
||
entry simply stops matching, the gate blocks the hook, kyberforge's `SessionStart` hook stops
|
||
deploying, and the apm install goes quietly stale — the exact failure ADR-0019 exists to end,
|
||
reintroduced through the mechanism meant to secure it. ADR-0019 records this as a live failure mode;
|
||
the release that shipped the hook hit it immediately.
|
||
|
||
`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 the **sixth and last alternation** in that hook's `exclude:` pattern, and the only one there
|
||
for a reason other than "generated manifest". Mind which number you are quoting: **six alternations,
|
||
expanding to sixteen real files** — 3 root marketplace manifests, 2 per plugin × 6 plugins, plus this
|
||
one.
|
||
|
||
`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
|
||
|
||
Exactly **two** pre-push hooks need the network, for one shared reason: root `apm.yml`'s
|
||
`marketplace.packages[]` contains exactly one remote entry — `mattpocock-skills`,
|
||
`source: mattpocock/skills` — and resolving it needs a `git ls-remote`.
|
||
|
||
| Hook | Offline failure |
|
||
|---|---|
|
||
| `apm-marketplace-check` (`always_run`, resolves every entry) | `No cached refs (offline)` |
|
||
| `apm-pack-check-clean` (re-resolves the same entry) | `Error: Git network timeout during ls-remote` |
|
||
|
||
Pinning the entry to an exact version does **not** remove the call — an exact pin still ls-remotes.
|
||
`--offline` rescues neither.
|
||
|
||
To push without a network, skip both using pre-commit's own mechanism:
|
||
|
||
```
|
||
SKIP=apm-marketplace-check,apm-pack-check-clean git push
|
||
```
|
||
|
||
**Skip those two alone.** Verified under `unshare -rn`: the other twelve pre-push hooks pass offline
|
||
because they are real local checks. (`check-executables-allow-sync` landed after that run, but reads
|
||
two local manifests and makes no network call.) Adding any other hook to `SKIP` disarms it silently.
|
||
|
||
`apm-audit-ci` calls `apm` too but stays local: its org-policy discovery resolves nothing on this
|
||
remote *before* any network call, so it does not join the pair above.
|
||
|
||
---
|
||
|
||
## 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/0017-plugin-content-mirror-bridges-apm-to-host-discovery.md`,
|
||
`docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md`,
|
||
`docs/adr/0014-vale-prefilter-ships-from-the-plugin.md` — plugin content sync, apm-generated
|
||
manifests, committed Vale styles
|
||
- `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
|