From ee248ff5a5e660c17792eb0fe5219e64d3f99e6f Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Mon, 31 Aug 2026 08:02:44 +0000 Subject: [PATCH] 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. --- AGENTS.md | 2 +- docs/spec/gates.md | 156 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 02ed5da..32fd71d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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//.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. diff --git a/docs/spec/gates.md b/docs/spec/gates.md index 7fb8ea9..c00048a 100644 --- a/docs/spec/gates.md +++ b/docs/spec/gates.md @@ -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.