docs: describe the boundary check's new outcomes and de-pin stale counts
The gate spec still described the routing-target check as pass/fail against a single-arrow clause. It now documents the three outcomes, the hand-invocation carve-out and the one-arrow rule, so a contributor hitting a SUGGESTION can tell whether it is a real defect or accepted phrasing. Both files cited skill counts and source line numbers that go stale on the next edit and were already wrong; those citations are removed rather than refreshed.
This commit is contained in:
@@ -36,7 +36,7 @@ Fall back to raw shell only when no skill covers it.
|
||||
- **Do not add repo-owned keys to `.claude/settings.json`.** apm treats it as its own deployed artifact and `apm audit --ci` replays the install and diffs, so anything apm would not have written is permanent drift that fails the `apm-audit-ci` pre-push hook. A hook you want here is authored in `plugins/<name>/.apm/hooks/` and deployed by apm, never hand-written into that file. The `SessionStart` entry already in it is exactly that: kyberforge authors it in `plugins/kyberforge/.apm/hooks/hooks.json` and apm merges it in, so it is apm's own output, it is what the replay expects, and it belongs in the commit — do not strip it (ADR-0019). Machine-specific settings go in the gitignored `.claude/settings.local.json`; shared enforcement goes in `.pre-commit-config.yaml`.
|
||||
- **`apm.lock.yaml` turning up modified is expected, not a bug.** kyberforge's `SessionStart` hook runs `apm outdated` at startup and `apm update --yes` when something is behind, which rewrites the lock. Commit or discard it deliberately.
|
||||
- **A `.apm/` edit is not live in this session until it is pushed.** The six dependencies resolve from the holocron remote, unpinned against the default branch. `apm install` deploys from the lock; `apm update` is what re-resolves refs.
|
||||
- **The ADR-0020 skill gates ship hot, with no baseline — and the corpus is now clean.** All 39 skills clear both FAIL tiers: no description over 400 characters, no body over 900 words (counted body-only). Issue #99 retrofitted them plugin by plugin — `kyberforge` was the last plugin wave, followed by corpus-wide passes and two rounds of independent audit fixes. Because nothing is grandfathered, the gates now bite on first commit — a new skill, or an edit that pushes a description past 400, is blocked until it complies. **No routing target dangles**, and `tests/test-adr0020-targets.sh` pins that set as empty, so a new boundary clause naming a non-existent skill fails the suite rather than joining a backlog. Two blind spots survive: `skill-size-check` does not cover the Vale half, so `Kyberforge.CompositionNote` fires nowhere today but any new description can reintroduce it; and every `references/` file is unlinted — which matters because the contract's own remedy is to move prose *into* `references/`, out of the prose gate's reach. That blind spot has **two** independent causes and closing either alone changes nothing: the `Kyberforge` style is scoped `[**/SKILL.md]`, *and* the `vale-audit-prefilter-skill` hook filters on `files: '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$'`, so a reference file is never handed to Vale whatever the style says (#117). Check both gates: `pre-commit run --all-files`.
|
||||
- **The ADR-0020 skill gates ship hot, with no baseline — and the corpus is now clean.** All 39 skills clear both FAIL tiers: no description over 400 characters, no body over 900 words (counted body-only). Retrofitted plugin by plugin under #99 (see `docs/spec/gates.md`). Because nothing is grandfathered, the gates now bite on first commit — a new skill, or an edit that pushes a description past 400, is blocked until it complies. **No routing target dangles**, and `tests/test-adr0020-targets.sh` pins that set as empty, so a new boundary clause naming a non-existent skill fails the suite rather than joining a backlog. Two blind spots survive: `skill-size-check` does not cover the Vale half, so `Kyberforge.CompositionNote` fires nowhere today but any new description can reintroduce it; and every `references/` file is unlinted — which matters because the contract's own remedy is to move prose *into* `references/`, out of the prose gate's reach. That blind spot has **two** independent causes and closing either alone changes nothing: the `Kyberforge` style is scoped `[**/SKILL.md]` (the cause #117 records), *and* the `vale-audit-prefilter-skill` hook filters on `files: '^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$'`, so a reference file is never handed to Vale whatever the style says. Check both gates: `pre-commit run --all-files`.
|
||||
- **Run `bash tests/run-tests.sh --strict` before considering any change done.** Keep the flag: without it a suite whose dependency is missing exits 77 and is counted SKIPPED rather than failed, so the run goes green having verified less than it claims.
|
||||
- **Before pushing, rehearse the gate locally:** `pre-commit run --hook-stage pre-push --all-files`. It runs the 14 pre-push hooks this repo authors itself plus pre-commit's 2 `meta` hooks, so it prints 16; `check-release-needed` passes without checking anything, because it needs a real push to `main`. `docs/spec/gates.md` reconciles both.
|
||||
- **Pushing without a network** needs `SKIP=apm-marketplace-check,apm-pack-check-clean git push` — those two resolve a remote marketplace entry via `git ls-remote`. Skip only those two; the rest are real local checks, and adding one to `SKIP` disarms it silently.
|
||||
|
||||
@@ -141,7 +141,7 @@ A boundary-clause target that resolves to no skill or agent has **three** possib
|
||||
| Verdict | When |
|
||||
|---|---|
|
||||
| **SUGGESTION** — the default | the target does not resolve and neither promotion condition below holds |
|
||||
| **blocking ERROR** | the target is **terminal** (not a compound modifier) **and** either written in route notation (`/name` for any name; `-> name` only when the name is hyphenated — see the gap below) **or** corroborated by another target in the same sentence that *does* resolve |
|
||||
| **blocking ERROR** | the target is **terminal** (not a compound modifier) **and** either written in route notation (`/name` for any name; a bare `-> name` only when the name is hyphenated, a backticked `` -> `name` `` for any — see the gap below) **or** 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
|
||||
@@ -149,14 +149,25 @@ tool, a file format or an English compound as a route: "pre-commit hooks" is pro
|
||||
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.
|
||||
|
||||
**Known gap: the arrow form only works for hyphenated names.** Target extraction is built on
|
||||
`NAME_HYPH` (`scripts/skill-size-check.sh:543`), which requires at least one hyphen, and
|
||||
`ARROW_BOUNDARY` (`:561`) inherits that. So `-> gitea-prs` is extracted and checked, while
|
||||
`-> triage` is not extracted at all — no ERROR, no SUGGESTION, exit 0. The unicode arrow `→` is not
|
||||
recognised in either case. This makes the SUGGESTION's own advice unsafe for a single-word skill:
|
||||
taking it silences the finding rather than checking it. `/name` has no such restriction and is the
|
||||
form to prefer. Tracked as a defect; `tests/test-adr0020-targets.sh` has one arrow case and its
|
||||
target happens to be hyphenated, so nothing currently covers this.
|
||||
**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
|
||||
@@ -200,17 +211,109 @@ through `.claude/skills/` alone, so **the same commit measured 2 dangling target
|
||||
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, produces findings identical to the working tree — **26 description
|
||||
FAILs, 9 body FAILs, 2 dangling targets, 0 missing references, 58 SUGGESTIONs**.
|
||||
`.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
|
||||
|
||||
Three more, deterministic to measure but judgment to act on:
|
||||
Deterministic to measure, judgment to act on:
|
||||
|
||||
- a description with **no boundary clause at all**;
|
||||
- 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
|
||||
@@ -281,8 +384,10 @@ script.** `scripts/skill-size-check.sh` applies its body gate to whatever path i
|
||||
bash scripts/skill-size-check.sh plugins/*/.apm/agents/*.agent.md
|
||||
```
|
||||
|
||||
exits 1 today with 900-word body FAILs on `git-orchestrate` (933), `gitea-orchestrate` (1,199) and
|
||||
`apm-orchestrate` (1,113). Agent files escape only because the hook definitions filter on `SKILL.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.
|
||||
@@ -293,13 +398,30 @@ knows the difference; doing so silently enforces a gate ADR-0020 declines to set
|
||||
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, followed by two corpus-wide passes.
|
||||
`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; 31 SUGGESTIONs |
|
||||
| `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.
|
||||
|
||||
Reference in New Issue
Block a user