From db5a42641690ef429e767ed0341904fe3d18b491 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Mon, 31 Aug 2026 08:01:07 +0000 Subject: [PATCH] fix(gates): make the ADR-0020 boundary check parse what skills actually write The routing-target check understood only a single-arrow clause naming a bare skill, so most real boundary prose was silently skipped rather than verified. Two of those silences were fail-open: an unrecognised token following a target dropped that target from the check entirely, and a skill directory with no SKILL.md still resolved as a valid routing target, so a broken route passed. Multi-target arrow clauses now draw a SUGGESTION instead of being ignored, hand-invocation phrasing is carved out so it is not read as a route, and a dotted filename parses into a new `unparsed` status rather than disappearing. Three test fixtures had been relying on the SKILL.md-less directory resolving as a target; they are corrected alongside the check. Addresses #107, #108, #110. --- .../skills/agent-audit/scripts/validate.sh | 245 ++++++++++++++- .../skills/skill-audit/scripts/validate.sh | 246 ++++++++++++++- .../skills/skill-audit/tests/validate.bats | 177 ++++++++++- .../skills/agent-audit/scripts/validate.sh | 245 ++++++++++++++- .../skills/skill-audit/scripts/validate.sh | 246 ++++++++++++++- scripts/skill-size-check.sh | 291 ++++++++++++++++-- tests/test-adr0020-targets.sh | 199 +++++++++++- tests/test-skill-size-check.sh | 129 +++++++- 8 files changed, 1688 insertions(+), 90 deletions(-) diff --git a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh index b13a210..c24f739 100755 --- a/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/agent-audit/scripts/validate.sh @@ -253,7 +253,15 @@ def _collect_package(pkg_dir, names): safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): for path in glob.glob(os.path.join(safe_dir, sub)): - names.add(os.path.basename(path.rstrip('/')).lower()) + # A directory is a skill only if it HOLDS a SKILL.md. An empty + # leftover — a deleted skill whose directory survived, a scaffolding + # stub, an editor's stray mkdir — is untracked by git, so it exists + # on the machine that made it and nowhere else. Counting it made a + # boundary target resolve locally and dangle in a fresh clone: the + # same install-dependence the deployed-tree rule above exists to + # remove, arriving through a different door. + if os.path.isfile(os.path.join(path, 'SKILL.md')): + names.add(os.path.basename(path.rstrip('/')).lower()) for sub in ('.apm/agents/*.md', 'agents/*.md'): for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) @@ -557,12 +565,33 @@ ROUTE_ANY = re.compile(r"\b%s\s+(?:the\s+|an?\s+)?%s" % (ROUTE_VERB, ANY_TARGET) CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +# CLAUSE_BODY is what may sit between `Not` and the arrow, and it is NOT +# `[^.;]`. That class cannot cross a `.`, so every boundary clause naming a +# DOTTED FILENAME between the two — `.pre-commit-config.yaml`, `AGENTS.md`, +# `.vale.ini` — was invisible to both patterns below, and the two resulting +# failures were different sizes (issue #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. +# Three authors in two retrofit waves reworded a correct clause to satisfy +# the regex, one of them stripping the very filename that discriminates the +# skill from its neighbour. +# * 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 failure modes. +# A dot inside a filename is followed by a non-space; a sentence-ending dot is +# followed by whitespace or by end of string. So the class admits a `.` only +# when the next character is not whitespace, which crosses `AGENTS.md` and +# still stops at a real sentence end. +CLAUSE_BODY = r"(?:[^.;]|\.(?=\S))" +ARROW_BOUNDARY = re.compile( + r"\bnot\b%s*?(?:->|→)\s*(%s)\b" % (CLAUSE_BODY, NAME_HYPH), re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # A boundary clause takes two shapes and BOTH count: the prose markers, and # ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b%s*?(?:->|→)" % CLAUSE_BODY, re.I) # Sentence boundaries decide the CORROBORATION scope above, so getting one wrong # is not cosmetic — it moves a target between SUGGESTION and blocking ERROR. Two # shapes common in these descriptions defeat the naive "period, space, capital" @@ -582,9 +611,17 @@ BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) # a lowercase letter. Verified zero-delta on the current corpus (37 ERROR / 58 # SUGGESTION / 2 dangling before and after) — this protects the descriptions # issue #99 is about to rewrite, not the ones already measured. +# re.I here too, and NOT as a tidy-up: this was the one pattern in the file +# built without it, contradicting the uniformity note on CONT_*/ARROW_* above. +# Without the flag `E.g.` and `I.e.` — the sentence-initial spellings, which is +# where an abbreviation most often lands — matched none of the lookbehinds, so +# the clause split at the abbreviation, the corroborating target was stranded on +# the far side of the cut, and a genuinely dangling target silently demoted from +# blocking ERROR to SUGGESTION. That is the OVER-SPLIT failure described +# directly above, still live for exactly the capitalised half of the input. SENTENCE_SPLIT = re.compile( u'(? name` reached this function with + strict=True from its two call sites, but `/name` did not, so it fell to + _terminal() and a follower outside FOLLOWER_OK set may_dangle=False. The + target then reached unresolved_targets() unblockable — and, before the + companion fix there, unreported as well. `... use /no-such-skill + afterwards.` exited 0 in total silence, on the one form ADR-0020 offers an + author who wants a route checked unconditionally. + """ if not name: return + notation = _notation(text, start, arrow) + if strict is None and notation: + strict = True out.append((name, _terminal(text, end) if strict is None else strict, - _notation(text, start, arrow))) + notation)) def _scan(text, route_re, cont_re, out): @@ -706,6 +758,85 @@ def boundary_targets(description): return sorted({name for name, _, _ in _extract(description)}) +def _arrow_targets(description): + """Names extracted from ARROW notation specifically. + + Kept apart from boundary_targets() because the arrow form is the one shape + that ALWAYS names a target: ADR-0020's `Not -> `. A clause + written that way from which nothing could be extracted is a parse failure + that deserves its own message, and telling it apart needs the arrow targets + alone rather than every target in the description. + """ + out = [] + for sentence in SENTENCE_SPLIT.split(description): + for match in ARROW_MARKED.finditer(sentence): + name, _, _ = _first(match) + if name: + out.append(name) + for match in ARROW_BOUNDARY.finditer(sentence): + out.append(match.group(1)) + return out + + +def boundary_clause_status(description): + """'absent', 'unparsed' or 'present' — three outcomes, not two. + + Issue #110's standing request: the gate must distinguish "no boundary + clause" from "boundary clause I could not parse". Reporting the first for + the second sends the author hunting for a problem that is not there, and + three of them reworded a correct clause to satisfy a regex instead. + + 'unparsed' is the narrow, certain case: an ADR-0020 arrow clause was + detected and NO target came out of it. The arrow form always names one, so + zero targets means the name is written in a shape the extractor cannot see + — a single-word bare target (`Not X -> forge`, which has to be written + `` `forge` `` or `/forge`) is the live example, since single-word names are + deliberately not matchable bare. + + A PROSE clause yielding no target is NOT reported: "Do not use for anything + else" is a complete and legitimate boundary clause that names nowhere to go. + """ + if BOUNDARY_ARROW.search(description) and not _arrow_targets(description): + return 'unparsed' + if has_boundary_clause(description): + return 'present' + return 'absent' + + +def multi_target_arrow_clauses(description): + """[(first, second)] for arrow clauses naming more than one target. + + Issue #107: only the FIRST target after an arrow is resolved. The + conjunction continuation (CONT_*) is wired to the prose route verbs and + never to arrows, so `Not X -> a or b` resolved `a`, left `b` neither + resolved nor reported, and then printed "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. + + 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; rejecting costs nothing and makes the + one-arrow-per-target convention — already what every retrofitted gitea + skill does in practice — explicit instead of folkloric. The caller emits a + SUGGESTION telling the author to split. + """ + hits = [] + for sentence in SENTENCE_SPLIT.split(description): + matches = (list(ARROW_MARKED.finditer(sentence)) + + list(ARROW_BOUNDARY.finditer(sentence))) + for match in matches: + first, _, _ = _first(match) + if not first: + continue + cont = CONT_ANY.match(sentence, match.end()) + if not cont: + continue + second, _, _ = _first(cont) + if second: + hits.append((first, second)) + return hits + + def unresolved_targets(description, known): """Targets resolving to nothing, split into (blocking, reported). @@ -722,6 +853,17 @@ def unresolved_targets(description, known): Everything else is reported and left alone. `known` is the resolved universe from known_targets(); passing an empty set is not meaningful — callers check for that first and decline out loud instead. + + A NON-TERMINAL target is reported, never dropped. FOLLOWER_OK is a closed + whitelist of maybe eighty words, so the follower rule says "this token is + outside a list I keep" and not "this is prose" — and the old `continue` + turned that into invisibility at every tier. The gate then failed OPEN on + its own unfamiliarity: any target followed by a word nobody thought to + enumerate was neither blocked nor mentioned, so the check that did not run + said nothing about not running. The follower rule may withdraw the power to + BLOCK a commit — that is what it was added for, and the ATTRIBUTIVE USE note + above is the argument for it — but it may not withdraw visibility, which is + the same rule the corroboration tier already follows. """ blocking, reported = set(), set() for sentence in SENTENCE_SPLIT.split(description): @@ -730,7 +872,10 @@ def unresolved_targets(description, known): if normalize_target(name) in known} for name, may_dangle, notation in found: key = normalize_target(name) - if key in known or not may_dangle: + if key in known: + continue + if not may_dangle: + reported.add(name) continue if notation or (resolved - {key}): blocking.add(name) @@ -810,6 +955,47 @@ def description_value(fm_text): return re.sub(r'\s+', ' ', value).strip() +def hand_invoked(fm_text): + """True when the frontmatter marks this file as reached only by hand. + + `disable-model-invocation: true` removes a 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 give such a skill ONE plain human-facing sentence: + no trigger list, no boundary clause. No validator knew the field existed + (issue #108), so the boundary-clause SUGGESTION fired on exactly the shape + the contract mandates, and its remedy — "add a boundary clause 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. + + Only the ROUTING rules are lifted. The body word budget still applies: the + body is loaded on invocation like any other, and competes with the caller's + live conversation the same way. So does the 400-character description FAIL — + a hand-invoked description is not preloaded, but it is still the one line + the user reads when choosing from the `/` menu, and the ceiling is the + outlier stop rather than the style target. + + A parse failure returns False rather than raising. This is a MODIFIER on + other checks, not a check of its own: the frontmatter's validity is decided, + and failed, by description_value() on the same text, and raising a second + exception here would report one broken file twice with two different + diagnoses. + """ + try: + data = yaml.safe_load(fm_text) + except Exception: + return False + if not isinstance(data, dict): + return False + value = data.get('disable-model-invocation') + if isinstance(value, str): + # PyYAML already resolves the unquoted YAML 1.1 booleans, so this only + # catches a QUOTED "true" — which a host reads as truthy and which no + # gate should treat as opting back in to the routing rules. + return value.strip().lower() in ('true', 'yes', 'on') + return value is True + + # --- Body-shape checks (skills only; agents have no references/ dir) ------- # Deterministic and countable, so they are enforced here. Whether a given # gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why @@ -977,8 +1163,14 @@ def agent_description(fm, local_fname): f"not run — {local_fname}") return None -def check_description_budget(value, local_fname): - """ADR-0020 description gates — identical for every scope.""" +def check_description_budget(value, local_fname, by_hand=False): + """ADR-0020 description gates — identical for every scope. + + `by_hand` is ADR-0020's hand-invocation carve-out (issue #108): an agent + carrying `disable-model-invocation: true` is absent from the model-visible + listing, so the 250-character SUGGESTION — a routing-quality budget — has + no listing to apply to. The 400-character ceiling is unaffected. + """ if not value: return dlen = len(value) @@ -988,13 +1180,13 @@ def check_description_budget(value, local_fname): f"agent is invoked. Keep a trigger clause, at most one capability clause, " f"and a boundary clause; move capability enumeration, output-format detail, " f"composition notes and implementation detail to the body — {local_fname}") - elif dlen > DESC_SUGGEST_CHARS: + elif dlen > DESC_SUGGEST_CHARS and not by_hand: suggest(f"description is {dlen} chars — over the {DESC_SUGGEST_CHARS}-character " f"ADR-0020 target (hard fail at {DESC_MAX_CHARS}). The SUGGESTION tier is " f"what moves the corpus average; the FAIL tier only stops outliers " f"— {local_fname}") -def check_boundary(value, fpath, local_fname): +def check_boundary(value, fpath, local_fname, by_hand=False): """ADR-0020 boundary clause + resolvable boundary targets. agent-author's SKILL.md states that an agent's boundary targets must @@ -1011,10 +1203,31 @@ def check_boundary(value, fpath, local_fname): # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether # this particular agent warrants a boundary clause is judgment. All four # agents in this corpus currently lack one. - if not has_boundary_clause(value): + # + # THREE outcomes, not two: "no boundary clause" and "boundary clause I could + # not parse" are different findings (issue #110). And a hand-invoked agent is + # exempt from the clause altogether (issue #108) — the boundary-target + # resolution below still runs, because a target it DOES name should still + # resolve. + status = boundary_clause_status(value) if not by_hand else 'present' + if status == 'absent': suggest(f"description has no boundary clause — add the prose form (\"Do not use " f"for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " f"so the router knows where NOT to send this agent — {local_fname}") + elif status == 'unparsed': + suggest(f"description has an arrow boundary clause (\"Not X -> y\") from which no " + f"target could be read, so the dangling-target check did not run on it — " + f"the clause is PRESENT and unparsed, not missing. Most often the target " + f"is a single word, which is deliberately not matchable bare: write it as " + f"`name` or /name — {local_fname}") + if not by_hand: + # One arrow, one target: a second name after the same arrow is resolved + # by nothing and reported by nothing (issue #107). + for first, second in multi_target_arrow_clauses(value): + suggest(f"an arrow boundary clause names more than one target ('{first}', then " + f"'{second}') and only the first is resolved — the second is checked by " + f"nothing. Split it into one arrow per target: \"Not X -> {first}. " + f"Not Y -> {second}.\" — {local_fname}") targets = boundary_targets(value) if not targets: return @@ -1249,8 +1462,9 @@ def check_apm_agent_file(fpath, allowlist, stem): else: if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(folded, local_fname) - check_boundary(folded, fpath, local_fname) + by_hand = hand_invoked(fm) + check_description_budget(folded, local_fname, by_hand) + check_boundary(folded, fpath, local_fname, by_hand) # body — required, non-empty, no placeholder; same Copilot truncation risk # applies since this file compiles verbatim into a real Copilot file downstream. @@ -1345,8 +1559,9 @@ def check_file(fpath, file_provider): else: if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(folded, local_fname) - check_boundary(folded, fpath, local_fname) + by_hand = hand_invoked(fm) + check_description_budget(folded, local_fname, by_hand) + check_boundary(folded, fpath, local_fname, by_hand) # body if not body.strip(): diff --git a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh index e169390..a871fe2 100755 --- a/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/.apm/skills/skill-audit/scripts/validate.sh @@ -179,7 +179,15 @@ def _collect_package(pkg_dir, names): safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): for path in glob.glob(os.path.join(safe_dir, sub)): - names.add(os.path.basename(path.rstrip('/')).lower()) + # A directory is a skill only if it HOLDS a SKILL.md. An empty + # leftover — a deleted skill whose directory survived, a scaffolding + # stub, an editor's stray mkdir — is untracked by git, so it exists + # on the machine that made it and nowhere else. Counting it made a + # boundary target resolve locally and dangle in a fresh clone: the + # same install-dependence the deployed-tree rule above exists to + # remove, arriving through a different door. + if os.path.isfile(os.path.join(path, 'SKILL.md')): + names.add(os.path.basename(path.rstrip('/')).lower()) for sub in ('.apm/agents/*.md', 'agents/*.md'): for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) @@ -483,12 +491,33 @@ ROUTE_ANY = re.compile(r"\b%s\s+(?:the\s+|an?\s+)?%s" % (ROUTE_VERB, ANY_TARGET) CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +# CLAUSE_BODY is what may sit between `Not` and the arrow, and it is NOT +# `[^.;]`. That class cannot cross a `.`, so every boundary clause naming a +# DOTTED FILENAME between the two — `.pre-commit-config.yaml`, `AGENTS.md`, +# `.vale.ini` — was invisible to both patterns below, and the two resulting +# failures were different sizes (issue #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. +# Three authors in two retrofit waves reworded a correct clause to satisfy +# the regex, one of them stripping the very filename that discriminates the +# skill from its neighbour. +# * 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 failure modes. +# A dot inside a filename is followed by a non-space; a sentence-ending dot is +# followed by whitespace or by end of string. So the class admits a `.` only +# when the next character is not whitespace, which crosses `AGENTS.md` and +# still stops at a real sentence end. +CLAUSE_BODY = r"(?:[^.;]|\.(?=\S))" +ARROW_BOUNDARY = re.compile( + r"\bnot\b%s*?(?:->|→)\s*(%s)\b" % (CLAUSE_BODY, NAME_HYPH), re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # A boundary clause takes two shapes and BOTH count: the prose markers, and # ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b%s*?(?:->|→)" % CLAUSE_BODY, re.I) # Sentence boundaries decide the CORROBORATION scope above, so getting one wrong # is not cosmetic — it moves a target between SUGGESTION and blocking ERROR. Two # shapes common in these descriptions defeat the naive "period, space, capital" @@ -508,9 +537,17 @@ BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) # a lowercase letter. Verified zero-delta on the current corpus (37 ERROR / 58 # SUGGESTION / 2 dangling before and after) — this protects the descriptions # issue #99 is about to rewrite, not the ones already measured. +# re.I here too, and NOT as a tidy-up: this was the one pattern in the file +# built without it, contradicting the uniformity note on CONT_*/ARROW_* above. +# Without the flag `E.g.` and `I.e.` — the sentence-initial spellings, which is +# where an abbreviation most often lands — matched none of the lookbehinds, so +# the clause split at the abbreviation, the corroborating target was stranded on +# the far side of the cut, and a genuinely dangling target silently demoted from +# blocking ERROR to SUGGESTION. That is the OVER-SPLIT failure described +# directly above, still live for exactly the capitalised half of the input. SENTENCE_SPLIT = re.compile( u'(? name` reached this function with + strict=True from its two call sites, but `/name` did not, so it fell to + _terminal() and a follower outside FOLLOWER_OK set may_dangle=False. The + target then reached unresolved_targets() unblockable — and, before the + companion fix there, unreported as well. `... use /no-such-skill + afterwards.` exited 0 in total silence, on the one form ADR-0020 offers an + author who wants a route checked unconditionally. + """ if not name: return + notation = _notation(text, start, arrow) + if strict is None and notation: + strict = True out.append((name, _terminal(text, end) if strict is None else strict, - _notation(text, start, arrow))) + notation)) def _scan(text, route_re, cont_re, out): @@ -632,6 +684,85 @@ def boundary_targets(description): return sorted({name for name, _, _ in _extract(description)}) +def _arrow_targets(description): + """Names extracted from ARROW notation specifically. + + Kept apart from boundary_targets() because the arrow form is the one shape + that ALWAYS names a target: ADR-0020's `Not -> `. A clause + written that way from which nothing could be extracted is a parse failure + that deserves its own message, and telling it apart needs the arrow targets + alone rather than every target in the description. + """ + out = [] + for sentence in SENTENCE_SPLIT.split(description): + for match in ARROW_MARKED.finditer(sentence): + name, _, _ = _first(match) + if name: + out.append(name) + for match in ARROW_BOUNDARY.finditer(sentence): + out.append(match.group(1)) + return out + + +def boundary_clause_status(description): + """'absent', 'unparsed' or 'present' — three outcomes, not two. + + Issue #110's standing request: the gate must distinguish "no boundary + clause" from "boundary clause I could not parse". Reporting the first for + the second sends the author hunting for a problem that is not there, and + three of them reworded a correct clause to satisfy a regex instead. + + 'unparsed' is the narrow, certain case: an ADR-0020 arrow clause was + detected and NO target came out of it. The arrow form always names one, so + zero targets means the name is written in a shape the extractor cannot see + — a single-word bare target (`Not X -> forge`, which has to be written + `` `forge` `` or `/forge`) is the live example, since single-word names are + deliberately not matchable bare. + + A PROSE clause yielding no target is NOT reported: "Do not use for anything + else" is a complete and legitimate boundary clause that names nowhere to go. + """ + if BOUNDARY_ARROW.search(description) and not _arrow_targets(description): + return 'unparsed' + if has_boundary_clause(description): + return 'present' + return 'absent' + + +def multi_target_arrow_clauses(description): + """[(first, second)] for arrow clauses naming more than one target. + + Issue #107: only the FIRST target after an arrow is resolved. The + conjunction continuation (CONT_*) is wired to the prose route verbs and + never to arrows, so `Not X -> a or b` resolved `a`, left `b` neither + resolved nor reported, and then printed "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. + + 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; rejecting costs nothing and makes the + one-arrow-per-target convention — already what every retrofitted gitea + skill does in practice — explicit instead of folkloric. The caller emits a + SUGGESTION telling the author to split. + """ + hits = [] + for sentence in SENTENCE_SPLIT.split(description): + matches = (list(ARROW_MARKED.finditer(sentence)) + + list(ARROW_BOUNDARY.finditer(sentence))) + for match in matches: + first, _, _ = _first(match) + if not first: + continue + cont = CONT_ANY.match(sentence, match.end()) + if not cont: + continue + second, _, _ = _first(cont) + if second: + hits.append((first, second)) + return hits + + def unresolved_targets(description, known): """Targets resolving to nothing, split into (blocking, reported). @@ -648,6 +779,17 @@ def unresolved_targets(description, known): Everything else is reported and left alone. `known` is the resolved universe from known_targets(); passing an empty set is not meaningful — callers check for that first and decline out loud instead. + + A NON-TERMINAL target is reported, never dropped. FOLLOWER_OK is a closed + whitelist of maybe eighty words, so the follower rule says "this token is + outside a list I keep" and not "this is prose" — and the old `continue` + turned that into invisibility at every tier. The gate then failed OPEN on + its own unfamiliarity: any target followed by a word nobody thought to + enumerate was neither blocked nor mentioned, so the check that did not run + said nothing about not running. The follower rule may withdraw the power to + BLOCK a commit — that is what it was added for, and the ATTRIBUTIVE USE note + above is the argument for it — but it may not withdraw visibility, which is + the same rule the corroboration tier already follows. """ blocking, reported = set(), set() for sentence in SENTENCE_SPLIT.split(description): @@ -656,7 +798,10 @@ def unresolved_targets(description, known): if normalize_target(name) in known} for name, may_dangle, notation in found: key = normalize_target(name) - if key in known or not may_dangle: + if key in known: + continue + if not may_dangle: + reported.add(name) continue if notation or (resolved - {key}): blocking.add(name) @@ -736,6 +881,47 @@ def description_value(fm_text): return re.sub(r'\s+', ' ', value).strip() +def hand_invoked(fm_text): + """True when the frontmatter marks this file as reached only by hand. + + `disable-model-invocation: true` removes a 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 give such a skill ONE plain human-facing sentence: + no trigger list, no boundary clause. No validator knew the field existed + (issue #108), so the boundary-clause SUGGESTION fired on exactly the shape + the contract mandates, and its remedy — "add a boundary clause 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. + + Only the ROUTING rules are lifted. The body word budget still applies: the + body is loaded on invocation like any other, and competes with the caller's + live conversation the same way. So does the 400-character description FAIL — + a hand-invoked description is not preloaded, but it is still the one line + the user reads when choosing from the `/` menu, and the ceiling is the + outlier stop rather than the style target. + + A parse failure returns False rather than raising. This is a MODIFIER on + other checks, not a check of its own: the frontmatter's validity is decided, + and failed, by description_value() on the same text, and raising a second + exception here would report one broken file twice with two different + diagnoses. + """ + try: + data = yaml.safe_load(fm_text) + except Exception: + return False + if not isinstance(data, dict): + return False + value = data.get('disable-model-invocation') + if isinstance(value, str): + # PyYAML already resolves the unquoted YAML 1.1 booleans, so this only + # catches a QUOTED "true" — which a host reads as truthy and which no + # gate should treat as opting back in to the routing rules. + return value.strip().lower() in ('true', 'yes', 'on') + return value is True + + # --- Body-shape checks (skills only; agents have no references/ dir) ------- # Deterministic and countable, so they are enforced here. Whether a given # gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why @@ -909,6 +1095,15 @@ except FrontmatterError as exc: dir_name = os.path.basename(skill_dir) +# ADR-0020's hand-invocation carve-out (issue #108). `disable-model-invocation: +# true` takes the skill out of the model-visible listing entirely, so the +# trigger/capability/boundary rules and the 250-character routing target do not +# apply to it — the audit's own references/description-quality.md Step 0 says +# so, and until this line existed no check here knew the field existed. What the +# flag does NOT lift: the body word budget and the 400-character description +# ceiling. See the shared resolver's hand_invoked(). +by_hand = hand_invoked(fm) + # --- Checks --- # name present @@ -1023,10 +1218,13 @@ if desc: f"skill is invoked. Keep a trigger clause, at most one capability clause, " f"and a boundary clause; move capability enumeration, output-format detail, " f"composition notes and implementation detail to the body or README.md") - elif dlen > DESC_SUGGEST_CHARS: + elif dlen > DESC_SUGGEST_CHARS and not by_hand: suggest(f"description is {dlen} chars — over the {DESC_SUGGEST_CHARS}-character " f"ADR-0020 target (hard fail at {DESC_MAX_CHARS}). The SUGGESTION tier is " f"what moves the corpus average; the FAIL tier only stops outliers") + elif by_hand: + ok(f"description length {dlen} chars (hand-invoked: the {DESC_SUGGEST_CHARS}-character " + f"routing target does not apply, the {DESC_MAX_CHARS}-character ceiling still does)") else: ok(f"description length {dlen} chars (ADR-0020 target: {DESC_SUGGEST_CHARS})") @@ -1080,13 +1278,41 @@ if gotchas is not None: # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether # this particular skill warrants a boundary clause is judgment. Both accepted # shapes count — the prose markers and the compressed `Not -> `. -if desc: - if has_boundary_clause(desc): +# +# THREE outcomes, not two: "no boundary clause" and "boundary clause I could not +# parse" are different findings, and reporting the first for the second sends +# the author hunting for a problem that is not there (issue #110). +# +# Skipped entirely for a hand-invoked skill — the contract gives it one plain +# sentence with no boundary clause, so the finding would be wrong and its remedy +# names a router that cannot see the skill (issue #108). +if desc and by_hand: + ok("hand-invoked (disable-model-invocation) — the boundary-clause and trigger " + "rules do not apply; audited as one plain human-facing sentence") +elif desc: + status = boundary_clause_status(desc) + if status == 'present': ok("description has a boundary clause") - else: + elif status == 'absent': suggest("description has no boundary clause — add the prose form (\"Do not use " "for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " "so the router knows where NOT to send this skill") + else: + suggest("description has an arrow boundary clause (\"Not X -> y\") from which no " + "target could be read, so the dangling-target check did not run on it — " + "the clause is PRESENT and unparsed, not missing. Most often the target is " + "a single word, which is deliberately not matchable bare because " + "`research`, `triage` and `forge` are all ordinary English: write it as " + "`name` or /name") + # One arrow, one target. A second name after the same arrow is resolved by + # nothing and reported by nothing, so the clause claims coverage it does not + # have and this script printed "1 of 1 boundary target(s) resolve" on a + # clause naming two (issue #107). + for first, second in multi_target_arrow_clauses(desc): + suggest(f"an arrow boundary clause names more than one target ('{first}', then " + f"'{second}') and only the first is resolved — the second is checked by " + f"nothing. Split it into one arrow per target: \"Not X -> {first}. " + f"Not Y -> {second}.\"") # --- ADR-0020: resolvable boundary targets --------------------------------- # The resolution universe comes from the SKILL's own location: the authoring diff --git a/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats b/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats index f63d799..d41c364 100755 --- a/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats +++ b/plugins/kyberforge/.apm/skills/skill-audit/tests/validate.bats @@ -70,13 +70,23 @@ PY # this repo's live skills. Echoes the subject skill's directory. # # /plugins/fixture-plugin/.apm/skills//SKILL.md - # /plugins/fixture-plugin/.apm/skills/fixture-sibling-skill/ + # /plugins/fixture-plugin/.apm/skills/fixture-sibling-skill/SKILL.md # /plugins/fixture-plugin/.apm/agents/fixture-sibling-agent.agent.md + # + # The sibling gets a real SKILL.md, and that is load-bearing rather than + # tidiness: a directory under skills/ is a resolvable name only when it + # HOLDS one. An empty leftover directory is untracked by git, so counting + # one made a target resolve on the machine that made it and dangle in a + # fresh clone. This helper used to mkdir the sibling and write nothing into + # it, so the corroborator every blocking-tier test depends on silently + # stopped resolving the moment that rule was enforced. make_fixture_tree() { local root="$1" subject="$2" local apm="$root/plugins/fixture-plugin/.apm" mkdir -p "$apm/skills/$subject" "$apm/skills/fixture-sibling-skill" "$apm/agents" touch "$apm/agents/fixture-sibling-agent.agent.md" + make_sized_skill "$apm/skills/fixture-sibling-skill" \ + "Use when doing the other thing. Do not use for anything else." 10 echo "$apm/skills/$subject" } } @@ -568,3 +578,168 @@ EOF assert_output --partial "boundary-target resolution DID NOT RUN" assert_output --partial "Unchecked target(s): some-other-skill" } + +# --------------------------------------------------------------------------- +# ADR-0020 — the hand-invocation carve-out (issue #108) +# +# A skill carrying `disable-model-invocation: true` is absent from the +# model-visible listing entirely: not preloaded, and the Skill tool refuses to +# call it. Its description is never matched against user intent, so +# references/description-quality.md Step 0 gives it ONE plain human-facing +# sentence — no trigger list, no boundary clause — and calls a +# missing-boundary-clause finding on such a skill "a wrong finding, not a strict +# one". Until this ran, nothing here knew the field existed, so the audit +# reported exactly the shape its own rubric mandates, with advice naming a +# router that cannot see the skill. +# +# The carve-out is narrow. Both size gates are unaffected and both are pinned +# below: the body is loaded on invocation like any other body, and the +# 400-character ceiling is an outlier stop rather than a routing budget. +# --------------------------------------------------------------------------- + +# Helper: a skill directory carrying `disable-model-invocation: true`. +make_hand_invoked_skill() { + local dir="$1" desc="$2" body_words="$3" + local name + name="$(basename "$dir")" + mkdir -p "$dir" + { + echo "---" + echo "name: $name" + echo "description: $desc" + echo "disable-model-invocation: true" + echo "---" + echo "" + python3 -c "print(' '.join(['word'] * $body_words))" + } > "$dir/SKILL.md" +} + +@test "ADR-0020: a hand-invoked skill is not asked for a boundary clause" { + local skill="$TMPDIR/my-skill" + make_hand_invoked_skill "$skill" \ + "Tell the agent to zoom out and give broader context or a higher level perspective." 10 + run bash "$SCRIPT" "$skill" + assert_success + refute_output --partial "has no boundary clause" + assert_output --partial "hand-invoked" +} + +@test "ADR-0020: the SAME description without the flag IS asked for a boundary clause" { + # The control. Without it the case above is satisfied by an audit that + # stopped checking boundary clauses altogether. + local skill="$TMPDIR/my-skill" + make_sized_skill "$skill" \ + "Tell the agent to zoom out and give broader context or a higher level perspective." 10 + run bash "$SCRIPT" "$skill" + assert_success + assert_output --partial "has no boundary clause" +} + +@test "ADR-0020: a hand-invoked skill is exempt from the 250-character description target" { + local skill="$TMPDIR/my-skill" + make_hand_invoked_skill "$skill" \ + "$(python3 -c "print('Tell the agent to zoom out. ' + 'x' * 273)")" 10 + run bash "$SCRIPT" "$skill" + assert_success + refute_output --partial "over the 250-character" +} + +@test "ADR-0020: a hand-invoked description over 400 chars still FAILS" { + # The half the carve-out does NOT lift. 400 is an outlier stop, not a + # routing-quality target: a hand-invoked description is still the one line + # the user reads when choosing from the `/` menu. + local skill="$TMPDIR/my-skill" + make_hand_invoked_skill "$skill" \ + "$(python3 -c "print('Tell the agent to zoom out. ' + 'x' * 374)")" 10 + run bash "$SCRIPT" "$skill" + assert_failure + assert_output --partial "400-character" +} + +@test "ADR-0020: a hand-invoked body over 900 words still FAILS" { + # The body is loaded on invocation exactly like any other body and competes + # with the caller's live conversation the same way, so no body tier moves. + local skill="$TMPDIR/my-skill" + make_hand_invoked_skill "$skill" "Tell the agent to zoom out." 901 + run bash "$SCRIPT" "$skill" + assert_failure + assert_output --partial "900-word" +} + +# --------------------------------------------------------------------------- +# ADR-0020 — one arrow, one target (issue #107) +# +# Only the FIRST target after an arrow was resolved: the conjunction +# continuation is wired to the prose route verbs and never to arrows. So this +# script printed "1 of 1 boundary target(s) resolve" on a clause naming two, +# and the second was resolved by nothing and reported by nothing. A typo in it +# shipped through a green gate. The shape is now rejected rather than the +# extractor widened. +# --------------------------------------------------------------------------- + +@test "ADR-0020: an arrow clause naming two targets is reported, not silently half-checked" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + # A bare `Not ... ->` sentence carries no BOUNDARY_MARKER, so the backtick + # sweep does not run and the second target is invisible to every other rule + # in the resolver — this is the exact shape #107 measured. + make_sized_skill "$skill" "Use when doing the thing. Not the other thing -> \`fixture-sibling-skill\` or \`fixture-missing-second\`." 10 + run bash "$SCRIPT" "$skill" + assert_success + assert_output --partial "names more than one target" +} + +@test "ADR-0020: one arrow per target — the convention the suggestion asks for — is silent" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + make_sized_skill "$skill" "Use when doing the thing. Not the other thing -> \`fixture-sibling-skill\`." 10 + run bash "$SCRIPT" "$skill" + assert_success + refute_output --partial "names more than one target" +} + +# --------------------------------------------------------------------------- +# ADR-0020 — a dotted filename in a boundary clause (issue #110) +# +# `[^.;]` could not cross the `.` in `AGENTS.md`, so a clause naming a dotted +# file between "Not" and the arrow was invisible. With a backticked target that +# was a MISDIAGNOSIS — "no boundary clause" reported on a clause that was +# present and working. With a BARE target it was worse: the target was never +# extracted, so the dangling check silently did not run on it. +# --------------------------------------------------------------------------- + +@test "ADR-0020: a boundary clause naming a dotted filename is not reported as missing" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + make_sized_skill "$skill" "Use when doing the thing. Not AGENTS.md -> \`fixture-sibling-skill\`." 10 + run bash "$SCRIPT" "$skill" + assert_success + refute_output --partial "has no boundary clause" + assert_output --partial "description has a boundary clause" +} + +@test "ADR-0020: a BARE target after a dotted filename is extracted and checked" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + # The silent half of #110: this clause produced no target at all, so it was + # neither resolved nor reported — a route to a non-existent skill shipping + # through a green gate with no finding of any kind. + make_sized_skill "$skill" "Use when doing the thing. Not AGENTS.md -> fixture-missing-dotted." 10 + run bash "$SCRIPT" "$skill" + assert_failure + assert_output --partial "routes to 'fixture-missing-dotted'" +} + +@test "ADR-0020: an arrow clause yielding no target is reported as unparsed, not as missing" { + local skill + skill="$(make_fixture_tree "$TMPDIR/tree" "my-skill")" + # A single-word target is deliberately not matchable bare, because + # `research`, `triage` and `forge` are all skill names AND ordinary English. + # The clause is present; saying it is missing sends the author to add a + # second copy of a clause that is already there. + make_sized_skill "$skill" "Use when doing the thing. Not the other thing -> forge." 10 + run bash "$SCRIPT" "$skill" + assert_success + refute_output --partial "has no boundary clause" + assert_output --partial "no target could be read" +} diff --git a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh index b13a210..c24f739 100755 --- a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh @@ -253,7 +253,15 @@ def _collect_package(pkg_dir, names): safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): for path in glob.glob(os.path.join(safe_dir, sub)): - names.add(os.path.basename(path.rstrip('/')).lower()) + # A directory is a skill only if it HOLDS a SKILL.md. An empty + # leftover — a deleted skill whose directory survived, a scaffolding + # stub, an editor's stray mkdir — is untracked by git, so it exists + # on the machine that made it and nowhere else. Counting it made a + # boundary target resolve locally and dangle in a fresh clone: the + # same install-dependence the deployed-tree rule above exists to + # remove, arriving through a different door. + if os.path.isfile(os.path.join(path, 'SKILL.md')): + names.add(os.path.basename(path.rstrip('/')).lower()) for sub in ('.apm/agents/*.md', 'agents/*.md'): for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) @@ -557,12 +565,33 @@ ROUTE_ANY = re.compile(r"\b%s\s+(?:the\s+|an?\s+)?%s" % (ROUTE_VERB, ANY_TARGET) CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +# CLAUSE_BODY is what may sit between `Not` and the arrow, and it is NOT +# `[^.;]`. That class cannot cross a `.`, so every boundary clause naming a +# DOTTED FILENAME between the two — `.pre-commit-config.yaml`, `AGENTS.md`, +# `.vale.ini` — was invisible to both patterns below, and the two resulting +# failures were different sizes (issue #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. +# Three authors in two retrofit waves reworded a correct clause to satisfy +# the regex, one of them stripping the very filename that discriminates the +# skill from its neighbour. +# * 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 failure modes. +# A dot inside a filename is followed by a non-space; a sentence-ending dot is +# followed by whitespace or by end of string. So the class admits a `.` only +# when the next character is not whitespace, which crosses `AGENTS.md` and +# still stops at a real sentence end. +CLAUSE_BODY = r"(?:[^.;]|\.(?=\S))" +ARROW_BOUNDARY = re.compile( + r"\bnot\b%s*?(?:->|→)\s*(%s)\b" % (CLAUSE_BODY, NAME_HYPH), re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # A boundary clause takes two shapes and BOTH count: the prose markers, and # ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b%s*?(?:->|→)" % CLAUSE_BODY, re.I) # Sentence boundaries decide the CORROBORATION scope above, so getting one wrong # is not cosmetic — it moves a target between SUGGESTION and blocking ERROR. Two # shapes common in these descriptions defeat the naive "period, space, capital" @@ -582,9 +611,17 @@ BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) # a lowercase letter. Verified zero-delta on the current corpus (37 ERROR / 58 # SUGGESTION / 2 dangling before and after) — this protects the descriptions # issue #99 is about to rewrite, not the ones already measured. +# re.I here too, and NOT as a tidy-up: this was the one pattern in the file +# built without it, contradicting the uniformity note on CONT_*/ARROW_* above. +# Without the flag `E.g.` and `I.e.` — the sentence-initial spellings, which is +# where an abbreviation most often lands — matched none of the lookbehinds, so +# the clause split at the abbreviation, the corroborating target was stranded on +# the far side of the cut, and a genuinely dangling target silently demoted from +# blocking ERROR to SUGGESTION. That is the OVER-SPLIT failure described +# directly above, still live for exactly the capitalised half of the input. SENTENCE_SPLIT = re.compile( u'(? name` reached this function with + strict=True from its two call sites, but `/name` did not, so it fell to + _terminal() and a follower outside FOLLOWER_OK set may_dangle=False. The + target then reached unresolved_targets() unblockable — and, before the + companion fix there, unreported as well. `... use /no-such-skill + afterwards.` exited 0 in total silence, on the one form ADR-0020 offers an + author who wants a route checked unconditionally. + """ if not name: return + notation = _notation(text, start, arrow) + if strict is None and notation: + strict = True out.append((name, _terminal(text, end) if strict is None else strict, - _notation(text, start, arrow))) + notation)) def _scan(text, route_re, cont_re, out): @@ -706,6 +758,85 @@ def boundary_targets(description): return sorted({name for name, _, _ in _extract(description)}) +def _arrow_targets(description): + """Names extracted from ARROW notation specifically. + + Kept apart from boundary_targets() because the arrow form is the one shape + that ALWAYS names a target: ADR-0020's `Not -> `. A clause + written that way from which nothing could be extracted is a parse failure + that deserves its own message, and telling it apart needs the arrow targets + alone rather than every target in the description. + """ + out = [] + for sentence in SENTENCE_SPLIT.split(description): + for match in ARROW_MARKED.finditer(sentence): + name, _, _ = _first(match) + if name: + out.append(name) + for match in ARROW_BOUNDARY.finditer(sentence): + out.append(match.group(1)) + return out + + +def boundary_clause_status(description): + """'absent', 'unparsed' or 'present' — three outcomes, not two. + + Issue #110's standing request: the gate must distinguish "no boundary + clause" from "boundary clause I could not parse". Reporting the first for + the second sends the author hunting for a problem that is not there, and + three of them reworded a correct clause to satisfy a regex instead. + + 'unparsed' is the narrow, certain case: an ADR-0020 arrow clause was + detected and NO target came out of it. The arrow form always names one, so + zero targets means the name is written in a shape the extractor cannot see + — a single-word bare target (`Not X -> forge`, which has to be written + `` `forge` `` or `/forge`) is the live example, since single-word names are + deliberately not matchable bare. + + A PROSE clause yielding no target is NOT reported: "Do not use for anything + else" is a complete and legitimate boundary clause that names nowhere to go. + """ + if BOUNDARY_ARROW.search(description) and not _arrow_targets(description): + return 'unparsed' + if has_boundary_clause(description): + return 'present' + return 'absent' + + +def multi_target_arrow_clauses(description): + """[(first, second)] for arrow clauses naming more than one target. + + Issue #107: only the FIRST target after an arrow is resolved. The + conjunction continuation (CONT_*) is wired to the prose route verbs and + never to arrows, so `Not X -> a or b` resolved `a`, left `b` neither + resolved nor reported, and then printed "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. + + 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; rejecting costs nothing and makes the + one-arrow-per-target convention — already what every retrofitted gitea + skill does in practice — explicit instead of folkloric. The caller emits a + SUGGESTION telling the author to split. + """ + hits = [] + for sentence in SENTENCE_SPLIT.split(description): + matches = (list(ARROW_MARKED.finditer(sentence)) + + list(ARROW_BOUNDARY.finditer(sentence))) + for match in matches: + first, _, _ = _first(match) + if not first: + continue + cont = CONT_ANY.match(sentence, match.end()) + if not cont: + continue + second, _, _ = _first(cont) + if second: + hits.append((first, second)) + return hits + + def unresolved_targets(description, known): """Targets resolving to nothing, split into (blocking, reported). @@ -722,6 +853,17 @@ def unresolved_targets(description, known): Everything else is reported and left alone. `known` is the resolved universe from known_targets(); passing an empty set is not meaningful — callers check for that first and decline out loud instead. + + A NON-TERMINAL target is reported, never dropped. FOLLOWER_OK is a closed + whitelist of maybe eighty words, so the follower rule says "this token is + outside a list I keep" and not "this is prose" — and the old `continue` + turned that into invisibility at every tier. The gate then failed OPEN on + its own unfamiliarity: any target followed by a word nobody thought to + enumerate was neither blocked nor mentioned, so the check that did not run + said nothing about not running. The follower rule may withdraw the power to + BLOCK a commit — that is what it was added for, and the ATTRIBUTIVE USE note + above is the argument for it — but it may not withdraw visibility, which is + the same rule the corroboration tier already follows. """ blocking, reported = set(), set() for sentence in SENTENCE_SPLIT.split(description): @@ -730,7 +872,10 @@ def unresolved_targets(description, known): if normalize_target(name) in known} for name, may_dangle, notation in found: key = normalize_target(name) - if key in known or not may_dangle: + if key in known: + continue + if not may_dangle: + reported.add(name) continue if notation or (resolved - {key}): blocking.add(name) @@ -810,6 +955,47 @@ def description_value(fm_text): return re.sub(r'\s+', ' ', value).strip() +def hand_invoked(fm_text): + """True when the frontmatter marks this file as reached only by hand. + + `disable-model-invocation: true` removes a 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 give such a skill ONE plain human-facing sentence: + no trigger list, no boundary clause. No validator knew the field existed + (issue #108), so the boundary-clause SUGGESTION fired on exactly the shape + the contract mandates, and its remedy — "add a boundary clause 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. + + Only the ROUTING rules are lifted. The body word budget still applies: the + body is loaded on invocation like any other, and competes with the caller's + live conversation the same way. So does the 400-character description FAIL — + a hand-invoked description is not preloaded, but it is still the one line + the user reads when choosing from the `/` menu, and the ceiling is the + outlier stop rather than the style target. + + A parse failure returns False rather than raising. This is a MODIFIER on + other checks, not a check of its own: the frontmatter's validity is decided, + and failed, by description_value() on the same text, and raising a second + exception here would report one broken file twice with two different + diagnoses. + """ + try: + data = yaml.safe_load(fm_text) + except Exception: + return False + if not isinstance(data, dict): + return False + value = data.get('disable-model-invocation') + if isinstance(value, str): + # PyYAML already resolves the unquoted YAML 1.1 booleans, so this only + # catches a QUOTED "true" — which a host reads as truthy and which no + # gate should treat as opting back in to the routing rules. + return value.strip().lower() in ('true', 'yes', 'on') + return value is True + + # --- Body-shape checks (skills only; agents have no references/ dir) ------- # Deterministic and countable, so they are enforced here. Whether a given # gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why @@ -977,8 +1163,14 @@ def agent_description(fm, local_fname): f"not run — {local_fname}") return None -def check_description_budget(value, local_fname): - """ADR-0020 description gates — identical for every scope.""" +def check_description_budget(value, local_fname, by_hand=False): + """ADR-0020 description gates — identical for every scope. + + `by_hand` is ADR-0020's hand-invocation carve-out (issue #108): an agent + carrying `disable-model-invocation: true` is absent from the model-visible + listing, so the 250-character SUGGESTION — a routing-quality budget — has + no listing to apply to. The 400-character ceiling is unaffected. + """ if not value: return dlen = len(value) @@ -988,13 +1180,13 @@ def check_description_budget(value, local_fname): f"agent is invoked. Keep a trigger clause, at most one capability clause, " f"and a boundary clause; move capability enumeration, output-format detail, " f"composition notes and implementation detail to the body — {local_fname}") - elif dlen > DESC_SUGGEST_CHARS: + elif dlen > DESC_SUGGEST_CHARS and not by_hand: suggest(f"description is {dlen} chars — over the {DESC_SUGGEST_CHARS}-character " f"ADR-0020 target (hard fail at {DESC_MAX_CHARS}). The SUGGESTION tier is " f"what moves the corpus average; the FAIL tier only stops outliers " f"— {local_fname}") -def check_boundary(value, fpath, local_fname): +def check_boundary(value, fpath, local_fname, by_hand=False): """ADR-0020 boundary clause + resolvable boundary targets. agent-author's SKILL.md states that an agent's boundary targets must @@ -1011,10 +1203,31 @@ def check_boundary(value, fpath, local_fname): # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether # this particular agent warrants a boundary clause is judgment. All four # agents in this corpus currently lack one. - if not has_boundary_clause(value): + # + # THREE outcomes, not two: "no boundary clause" and "boundary clause I could + # not parse" are different findings (issue #110). And a hand-invoked agent is + # exempt from the clause altogether (issue #108) — the boundary-target + # resolution below still runs, because a target it DOES name should still + # resolve. + status = boundary_clause_status(value) if not by_hand else 'present' + if status == 'absent': suggest(f"description has no boundary clause — add the prose form (\"Do not use " f"for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " f"so the router knows where NOT to send this agent — {local_fname}") + elif status == 'unparsed': + suggest(f"description has an arrow boundary clause (\"Not X -> y\") from which no " + f"target could be read, so the dangling-target check did not run on it — " + f"the clause is PRESENT and unparsed, not missing. Most often the target " + f"is a single word, which is deliberately not matchable bare: write it as " + f"`name` or /name — {local_fname}") + if not by_hand: + # One arrow, one target: a second name after the same arrow is resolved + # by nothing and reported by nothing (issue #107). + for first, second in multi_target_arrow_clauses(value): + suggest(f"an arrow boundary clause names more than one target ('{first}', then " + f"'{second}') and only the first is resolved — the second is checked by " + f"nothing. Split it into one arrow per target: \"Not X -> {first}. " + f"Not Y -> {second}.\" — {local_fname}") targets = boundary_targets(value) if not targets: return @@ -1249,8 +1462,9 @@ def check_apm_agent_file(fpath, allowlist, stem): else: if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(folded, local_fname) - check_boundary(folded, fpath, local_fname) + by_hand = hand_invoked(fm) + check_description_budget(folded, local_fname, by_hand) + check_boundary(folded, fpath, local_fname, by_hand) # body — required, non-empty, no placeholder; same Copilot truncation risk # applies since this file compiles verbatim into a real Copilot file downstream. @@ -1345,8 +1559,9 @@ def check_file(fpath, file_provider): else: if PLACEHOLDER_RE.search(folded): fail(f"description contains unfilled FILL IN: placeholder — {local_fname}") - check_description_budget(folded, local_fname) - check_boundary(folded, fpath, local_fname) + by_hand = hand_invoked(fm) + check_description_budget(folded, local_fname, by_hand) + check_boundary(folded, fpath, local_fname, by_hand) # body if not body.strip(): diff --git a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh index e169390..a871fe2 100755 --- a/plugins/kyberforge/skills/skill-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/skill-audit/scripts/validate.sh @@ -179,7 +179,15 @@ def _collect_package(pkg_dir, names): safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): for path in glob.glob(os.path.join(safe_dir, sub)): - names.add(os.path.basename(path.rstrip('/')).lower()) + # A directory is a skill only if it HOLDS a SKILL.md. An empty + # leftover — a deleted skill whose directory survived, a scaffolding + # stub, an editor's stray mkdir — is untracked by git, so it exists + # on the machine that made it and nowhere else. Counting it made a + # boundary target resolve locally and dangle in a fresh clone: the + # same install-dependence the deployed-tree rule above exists to + # remove, arriving through a different door. + if os.path.isfile(os.path.join(path, 'SKILL.md')): + names.add(os.path.basename(path.rstrip('/')).lower()) for sub in ('.apm/agents/*.md', 'agents/*.md'): for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) @@ -483,12 +491,33 @@ ROUTE_ANY = re.compile(r"\b%s\s+(?:the\s+|an?\s+)?%s" % (ROUTE_VERB, ANY_TARGET) CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +# CLAUSE_BODY is what may sit between `Not` and the arrow, and it is NOT +# `[^.;]`. That class cannot cross a `.`, so every boundary clause naming a +# DOTTED FILENAME between the two — `.pre-commit-config.yaml`, `AGENTS.md`, +# `.vale.ini` — was invisible to both patterns below, and the two resulting +# failures were different sizes (issue #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. +# Three authors in two retrofit waves reworded a correct clause to satisfy +# the regex, one of them stripping the very filename that discriminates the +# skill from its neighbour. +# * 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 failure modes. +# A dot inside a filename is followed by a non-space; a sentence-ending dot is +# followed by whitespace or by end of string. So the class admits a `.` only +# when the next character is not whitespace, which crosses `AGENTS.md` and +# still stops at a real sentence end. +CLAUSE_BODY = r"(?:[^.;]|\.(?=\S))" +ARROW_BOUNDARY = re.compile( + r"\bnot\b%s*?(?:->|→)\s*(%s)\b" % (CLAUSE_BODY, NAME_HYPH), re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # A boundary clause takes two shapes and BOTH count: the prose markers, and # ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b%s*?(?:->|→)" % CLAUSE_BODY, re.I) # Sentence boundaries decide the CORROBORATION scope above, so getting one wrong # is not cosmetic — it moves a target between SUGGESTION and blocking ERROR. Two # shapes common in these descriptions defeat the naive "period, space, capital" @@ -508,9 +537,17 @@ BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) # a lowercase letter. Verified zero-delta on the current corpus (37 ERROR / 58 # SUGGESTION / 2 dangling before and after) — this protects the descriptions # issue #99 is about to rewrite, not the ones already measured. +# re.I here too, and NOT as a tidy-up: this was the one pattern in the file +# built without it, contradicting the uniformity note on CONT_*/ARROW_* above. +# Without the flag `E.g.` and `I.e.` — the sentence-initial spellings, which is +# where an abbreviation most often lands — matched none of the lookbehinds, so +# the clause split at the abbreviation, the corroborating target was stranded on +# the far side of the cut, and a genuinely dangling target silently demoted from +# blocking ERROR to SUGGESTION. That is the OVER-SPLIT failure described +# directly above, still live for exactly the capitalised half of the input. SENTENCE_SPLIT = re.compile( u'(? name` reached this function with + strict=True from its two call sites, but `/name` did not, so it fell to + _terminal() and a follower outside FOLLOWER_OK set may_dangle=False. The + target then reached unresolved_targets() unblockable — and, before the + companion fix there, unreported as well. `... use /no-such-skill + afterwards.` exited 0 in total silence, on the one form ADR-0020 offers an + author who wants a route checked unconditionally. + """ if not name: return + notation = _notation(text, start, arrow) + if strict is None and notation: + strict = True out.append((name, _terminal(text, end) if strict is None else strict, - _notation(text, start, arrow))) + notation)) def _scan(text, route_re, cont_re, out): @@ -632,6 +684,85 @@ def boundary_targets(description): return sorted({name for name, _, _ in _extract(description)}) +def _arrow_targets(description): + """Names extracted from ARROW notation specifically. + + Kept apart from boundary_targets() because the arrow form is the one shape + that ALWAYS names a target: ADR-0020's `Not -> `. A clause + written that way from which nothing could be extracted is a parse failure + that deserves its own message, and telling it apart needs the arrow targets + alone rather than every target in the description. + """ + out = [] + for sentence in SENTENCE_SPLIT.split(description): + for match in ARROW_MARKED.finditer(sentence): + name, _, _ = _first(match) + if name: + out.append(name) + for match in ARROW_BOUNDARY.finditer(sentence): + out.append(match.group(1)) + return out + + +def boundary_clause_status(description): + """'absent', 'unparsed' or 'present' — three outcomes, not two. + + Issue #110's standing request: the gate must distinguish "no boundary + clause" from "boundary clause I could not parse". Reporting the first for + the second sends the author hunting for a problem that is not there, and + three of them reworded a correct clause to satisfy a regex instead. + + 'unparsed' is the narrow, certain case: an ADR-0020 arrow clause was + detected and NO target came out of it. The arrow form always names one, so + zero targets means the name is written in a shape the extractor cannot see + — a single-word bare target (`Not X -> forge`, which has to be written + `` `forge` `` or `/forge`) is the live example, since single-word names are + deliberately not matchable bare. + + A PROSE clause yielding no target is NOT reported: "Do not use for anything + else" is a complete and legitimate boundary clause that names nowhere to go. + """ + if BOUNDARY_ARROW.search(description) and not _arrow_targets(description): + return 'unparsed' + if has_boundary_clause(description): + return 'present' + return 'absent' + + +def multi_target_arrow_clauses(description): + """[(first, second)] for arrow clauses naming more than one target. + + Issue #107: only the FIRST target after an arrow is resolved. The + conjunction continuation (CONT_*) is wired to the prose route verbs and + never to arrows, so `Not X -> a or b` resolved `a`, left `b` neither + resolved nor reported, and then printed "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. + + 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; rejecting costs nothing and makes the + one-arrow-per-target convention — already what every retrofitted gitea + skill does in practice — explicit instead of folkloric. The caller emits a + SUGGESTION telling the author to split. + """ + hits = [] + for sentence in SENTENCE_SPLIT.split(description): + matches = (list(ARROW_MARKED.finditer(sentence)) + + list(ARROW_BOUNDARY.finditer(sentence))) + for match in matches: + first, _, _ = _first(match) + if not first: + continue + cont = CONT_ANY.match(sentence, match.end()) + if not cont: + continue + second, _, _ = _first(cont) + if second: + hits.append((first, second)) + return hits + + def unresolved_targets(description, known): """Targets resolving to nothing, split into (blocking, reported). @@ -648,6 +779,17 @@ def unresolved_targets(description, known): Everything else is reported and left alone. `known` is the resolved universe from known_targets(); passing an empty set is not meaningful — callers check for that first and decline out loud instead. + + A NON-TERMINAL target is reported, never dropped. FOLLOWER_OK is a closed + whitelist of maybe eighty words, so the follower rule says "this token is + outside a list I keep" and not "this is prose" — and the old `continue` + turned that into invisibility at every tier. The gate then failed OPEN on + its own unfamiliarity: any target followed by a word nobody thought to + enumerate was neither blocked nor mentioned, so the check that did not run + said nothing about not running. The follower rule may withdraw the power to + BLOCK a commit — that is what it was added for, and the ATTRIBUTIVE USE note + above is the argument for it — but it may not withdraw visibility, which is + the same rule the corroboration tier already follows. """ blocking, reported = set(), set() for sentence in SENTENCE_SPLIT.split(description): @@ -656,7 +798,10 @@ def unresolved_targets(description, known): if normalize_target(name) in known} for name, may_dangle, notation in found: key = normalize_target(name) - if key in known or not may_dangle: + if key in known: + continue + if not may_dangle: + reported.add(name) continue if notation or (resolved - {key}): blocking.add(name) @@ -736,6 +881,47 @@ def description_value(fm_text): return re.sub(r'\s+', ' ', value).strip() +def hand_invoked(fm_text): + """True when the frontmatter marks this file as reached only by hand. + + `disable-model-invocation: true` removes a 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 give such a skill ONE plain human-facing sentence: + no trigger list, no boundary clause. No validator knew the field existed + (issue #108), so the boundary-clause SUGGESTION fired on exactly the shape + the contract mandates, and its remedy — "add a boundary clause 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. + + Only the ROUTING rules are lifted. The body word budget still applies: the + body is loaded on invocation like any other, and competes with the caller's + live conversation the same way. So does the 400-character description FAIL — + a hand-invoked description is not preloaded, but it is still the one line + the user reads when choosing from the `/` menu, and the ceiling is the + outlier stop rather than the style target. + + A parse failure returns False rather than raising. This is a MODIFIER on + other checks, not a check of its own: the frontmatter's validity is decided, + and failed, by description_value() on the same text, and raising a second + exception here would report one broken file twice with two different + diagnoses. + """ + try: + data = yaml.safe_load(fm_text) + except Exception: + return False + if not isinstance(data, dict): + return False + value = data.get('disable-model-invocation') + if isinstance(value, str): + # PyYAML already resolves the unquoted YAML 1.1 booleans, so this only + # catches a QUOTED "true" — which a host reads as truthy and which no + # gate should treat as opting back in to the routing rules. + return value.strip().lower() in ('true', 'yes', 'on') + return value is True + + # --- Body-shape checks (skills only; agents have no references/ dir) ------- # Deterministic and countable, so they are enforced here. Whether a given # gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why @@ -909,6 +1095,15 @@ except FrontmatterError as exc: dir_name = os.path.basename(skill_dir) +# ADR-0020's hand-invocation carve-out (issue #108). `disable-model-invocation: +# true` takes the skill out of the model-visible listing entirely, so the +# trigger/capability/boundary rules and the 250-character routing target do not +# apply to it — the audit's own references/description-quality.md Step 0 says +# so, and until this line existed no check here knew the field existed. What the +# flag does NOT lift: the body word budget and the 400-character description +# ceiling. See the shared resolver's hand_invoked(). +by_hand = hand_invoked(fm) + # --- Checks --- # name present @@ -1023,10 +1218,13 @@ if desc: f"skill is invoked. Keep a trigger clause, at most one capability clause, " f"and a boundary clause; move capability enumeration, output-format detail, " f"composition notes and implementation detail to the body or README.md") - elif dlen > DESC_SUGGEST_CHARS: + elif dlen > DESC_SUGGEST_CHARS and not by_hand: suggest(f"description is {dlen} chars — over the {DESC_SUGGEST_CHARS}-character " f"ADR-0020 target (hard fail at {DESC_MAX_CHARS}). The SUGGESTION tier is " f"what moves the corpus average; the FAIL tier only stops outliers") + elif by_hand: + ok(f"description length {dlen} chars (hand-invoked: the {DESC_SUGGEST_CHARS}-character " + f"routing target does not apply, the {DESC_MAX_CHARS}-character ceiling still does)") else: ok(f"description length {dlen} chars (ADR-0020 target: {DESC_SUGGEST_CHARS})") @@ -1080,13 +1278,41 @@ if gotchas is not None: # SUGGESTION, not FAIL: detecting the absence is deterministic, but whether # this particular skill warrants a boundary clause is judgment. Both accepted # shapes count — the prose markers and the compressed `Not -> `. -if desc: - if has_boundary_clause(desc): +# +# THREE outcomes, not two: "no boundary clause" and "boundary clause I could not +# parse" are different findings, and reporting the first for the second sends +# the author hunting for a problem that is not there (issue #110). +# +# Skipped entirely for a hand-invoked skill — the contract gives it one plain +# sentence with no boundary clause, so the finding would be wrong and its remedy +# names a router that cannot see the skill (issue #108). +if desc and by_hand: + ok("hand-invoked (disable-model-invocation) — the boundary-clause and trigger " + "rules do not apply; audited as one plain human-facing sentence") +elif desc: + status = boundary_clause_status(desc) + if status == 'present': ok("description has a boundary clause") - else: + elif status == 'absent': suggest("description has no boundary clause — add the prose form (\"Do not use " "for X — use `y` instead\") or ADR-0020's compressed form (\"Not X -> y\") " "so the router knows where NOT to send this skill") + else: + suggest("description has an arrow boundary clause (\"Not X -> y\") from which no " + "target could be read, so the dangling-target check did not run on it — " + "the clause is PRESENT and unparsed, not missing. Most often the target is " + "a single word, which is deliberately not matchable bare because " + "`research`, `triage` and `forge` are all ordinary English: write it as " + "`name` or /name") + # One arrow, one target. A second name after the same arrow is resolved by + # nothing and reported by nothing, so the clause claims coverage it does not + # have and this script printed "1 of 1 boundary target(s) resolve" on a + # clause naming two (issue #107). + for first, second in multi_target_arrow_clauses(desc): + suggest(f"an arrow boundary clause names more than one target ('{first}', then " + f"'{second}') and only the first is resolved — the second is checked by " + f"nothing. Split it into one arrow per target: \"Not X -> {first}. " + f"Not Y -> {second}.\"") # --- ADR-0020: resolvable boundary targets --------------------------------- # The resolution universe comes from the SKILL's own location: the authoring diff --git a/scripts/skill-size-check.sh b/scripts/skill-size-check.sh index 8ce8c7c..cf15c71 100755 --- a/scripts/skill-size-check.sh +++ b/scripts/skill-size-check.sh @@ -34,26 +34,44 @@ set -euo pipefail # as a proxy (Python's str.split(), the same primitive # skill-audit/scripts/validate.sh applies to these two constants; `wc -w` # disagrees with it on Unicode separators, which is why the awk pass that used -# to live in the loop below is gone). Measured over this repo's 39 in-scope SKILL.md files, characters per -# word runs min 5.97 / median 6.79 / mean 6.77 / max 7.22. At the standard -# ~4-characters-per-token English approximation that is 1.49 / 1.70 / 1.69 / -# 1.81 tokens per word. +# to live in the loop below is gone). +# +# THE MEASUREMENT BASIS, stated because the previous re-measure drifted onto a +# different one and the numbers moved without the prose noticing: characters +# per word is len(text) / len(text.split()) over the WHOLE FILE, whitespace +# included, on plugins/*/.apm/skills/*/SKILL.md. Counting only non-whitespace +# characters gives a materially lower figure (4.90 / 5.52 / 5.54 / 6.19 today) +# and is not the basis MAX_WORDS is calibrated against. +# +# Measured over this repo's 39 in-scope SKILL.md files (2026-08-31, after the +# ADR-0020 retrofit), characters per word runs min 5.93 / median 6.67 / mean +# 6.63 / max 7.34. At the standard ~4-characters-per-token English +# approximation that is 1.48 / 1.67 / 1.66 / 1.84 tokens per word. # # MAX_WORDS=2770 is therefore calibrated to the corpus WORST case rather than -# its median: 2770 words at the densest observed 7.22 chars/word is ~20,000 -# characters, or ~5,000 tokens at the 4-characters-per-token approximation. So -# what this gate guarantees is "under 5,000 tokens even for the densest prose +# its median: 2770 words at the densest observed 7.34 chars/word is ~20,300 +# characters, or ~5,090 tokens at the 4-characters-per-token approximation. So +# what this gate guarantees is "about 5,000 tokens even for the densest prose # the corpus has produced" — the earlier median-calibrated MAX_WORDS=2900 let -# such a file sit at exactly the ceiling and still spend ~5,240 tokens. A -# median-density file at 2770 words spends ~4,700 tokens, so typical prose -# gives up ~130 words of headroom to close that gap. The largest SKILL.md in -# the repo is 2,760 words whole-file (skill-author), twelve words under the -# ceiling — this is a gate two files have already grown into, not headroom. +# such a file sit at exactly the ceiling and spend ~5,320 tokens. A +# median-density file at 2770 words spends ~4,620 tokens, so typical prose +# gives up ~140 words of headroom to close that gap. Densest file today: +# git-commits at 7.34 chars/word. +# +# THE CORPUS IS NOWHERE NEAR THIS CEILING ANY MORE, and the note that used to +# stand here — "a gate two files have already grown into" — described the +# pre-retrofit corpus and is now wrong by a factor of three. The largest +# SKILL.md is write-docs at 914 whole-file words, then vale-run at 874; +# skill-author, the old high-water mark at 2,760, is down to 661. MAX_WORDS is +# a spec-conformance backstop with roughly 1,850 words of slack, and the gate +# that actually bites is ADR-0020's 900-word body budget below it. Do not read +# the two as redundant: they measure different spans, and a file can sit well +# inside one while failing the other. # # It is a one-sided proxy in the useful direction — nothing under the word # ceiling is wildly over the token ceiling — but it is not exact BPE -# tokenization and does not replace one. Re-measure the corpus before treating -# any of these numbers as still current. +# tokenization and does not replace one. Re-measure the corpus, on the basis +# stated above, before treating any of these numbers as still current. # # python3 AND PyYAML are required for the ADR-0020 half, and both are hard # dependencies rather than best-effort: python3 because pre-commit (which is how @@ -263,7 +281,15 @@ def _collect_package(pkg_dir, names): safe_dir = glob.escape(pkg_dir) for sub in ('.apm/skills/*/', 'skills/*/'): for path in glob.glob(os.path.join(safe_dir, sub)): - names.add(os.path.basename(path.rstrip('/')).lower()) + # A directory is a skill only if it HOLDS a SKILL.md. An empty + # leftover — a deleted skill whose directory survived, a scaffolding + # stub, an editor's stray mkdir — is untracked by git, so it exists + # on the machine that made it and nowhere else. Counting it made a + # boundary target resolve locally and dangle in a fresh clone: the + # same install-dependence the deployed-tree rule above exists to + # remove, arriving through a different door. + if os.path.isfile(os.path.join(path, 'SKILL.md')): + names.add(os.path.basename(path.rstrip('/')).lower()) for sub in ('.apm/agents/*.md', 'agents/*.md'): for path in glob.glob(os.path.join(safe_dir, sub)): base = os.path.basename(path) @@ -567,12 +593,33 @@ ROUTE_ANY = re.compile(r"\b%s\s+(?:the\s+|an?\s+)?%s" % (ROUTE_VERB, ANY_TARGET) CONT_MARKED = re.compile(r"\s*(?:or|and|/|,)\s*%s" % MARKED_TARGET, re.I) CONT_ANY = re.compile(r"\s*(?:or|and|/|,)\s*%s" % ANY_TARGET, re.I) ARROW_MARKED = re.compile(r"(?:->|→)\s*%s" % MARKED_TARGET, re.I) -ARROW_BOUNDARY = re.compile(r"\bnot\b[^.;]*?(?:->|→)\s*(%s)\b" % NAME_HYPH, re.I) +# CLAUSE_BODY is what may sit between `Not` and the arrow, and it is NOT +# `[^.;]`. That class cannot cross a `.`, so every boundary clause naming a +# DOTTED FILENAME between the two — `.pre-commit-config.yaml`, `AGENTS.md`, +# `.vale.ini` — was invisible to both patterns below, and the two resulting +# failures were different sizes (issue #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. +# Three authors in two retrofit waves reworded a correct clause to satisfy +# the regex, one of them stripping the very filename that discriminates the +# skill from its neighbour. +# * 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 failure modes. +# A dot inside a filename is followed by a non-space; a sentence-ending dot is +# followed by whitespace or by end of string. So the class admits a `.` only +# when the next character is not whitespace, which crosses `AGENTS.md` and +# still stops at a real sentence end. +CLAUSE_BODY = r"(?:[^.;]|\.(?=\S))" +ARROW_BOUNDARY = re.compile( + r"\bnot\b%s*?(?:->|→)\s*(%s)\b" % (CLAUSE_BODY, NAME_HYPH), re.I) BACKTICK = re.compile(r"`(%s)`" % NAME_HYPH, re.I) # A boundary clause takes two shapes and BOTH count: the prose markers, and # ADR-0020's compressed arrow form `Not -> `. BOUNDARY_MARKER = re.compile(r"\b(?:do\s+not|instead|rather\s+than|not\s+for)\b", re.I) -BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) +BOUNDARY_ARROW = re.compile(r"\bnot\b%s*?(?:->|→)" % CLAUSE_BODY, re.I) # Sentence boundaries decide the CORROBORATION scope above, so getting one wrong # is not cosmetic — it moves a target between SUGGESTION and blocking ERROR. Two # shapes common in these descriptions defeat the naive "period, space, capital" @@ -592,9 +639,17 @@ BOUNDARY_ARROW = re.compile(r"\bnot\b[^.;]*?(?:->|→)", re.I) # a lowercase letter. Verified zero-delta on the current corpus (37 ERROR / 58 # SUGGESTION / 2 dangling before and after) — this protects the descriptions # issue #99 is about to rewrite, not the ones already measured. +# re.I here too, and NOT as a tidy-up: this was the one pattern in the file +# built without it, contradicting the uniformity note on CONT_*/ARROW_* above. +# Without the flag `E.g.` and `I.e.` — the sentence-initial spellings, which is +# where an abbreviation most often lands — matched none of the lookbehinds, so +# the clause split at the abbreviation, the corroborating target was stranded on +# the far side of the cut, and a genuinely dangling target silently demoted from +# blocking ERROR to SUGGESTION. That is the OVER-SPLIT failure described +# directly above, still live for exactly the capitalised half of the input. SENTENCE_SPLIT = re.compile( u'(? name` reached this function with + strict=True from its two call sites, but `/name` did not, so it fell to + _terminal() and a follower outside FOLLOWER_OK set may_dangle=False. The + target then reached unresolved_targets() unblockable — and, before the + companion fix there, unreported as well. `... use /no-such-skill + afterwards.` exited 0 in total silence, on the one form ADR-0020 offers an + author who wants a route checked unconditionally. + """ if not name: return + notation = _notation(text, start, arrow) + if strict is None and notation: + strict = True out.append((name, _terminal(text, end) if strict is None else strict, - _notation(text, start, arrow))) + notation)) def _scan(text, route_re, cont_re, out): @@ -716,6 +786,85 @@ def boundary_targets(description): return sorted({name for name, _, _ in _extract(description)}) +def _arrow_targets(description): + """Names extracted from ARROW notation specifically. + + Kept apart from boundary_targets() because the arrow form is the one shape + that ALWAYS names a target: ADR-0020's `Not -> `. A clause + written that way from which nothing could be extracted is a parse failure + that deserves its own message, and telling it apart needs the arrow targets + alone rather than every target in the description. + """ + out = [] + for sentence in SENTENCE_SPLIT.split(description): + for match in ARROW_MARKED.finditer(sentence): + name, _, _ = _first(match) + if name: + out.append(name) + for match in ARROW_BOUNDARY.finditer(sentence): + out.append(match.group(1)) + return out + + +def boundary_clause_status(description): + """'absent', 'unparsed' or 'present' — three outcomes, not two. + + Issue #110's standing request: the gate must distinguish "no boundary + clause" from "boundary clause I could not parse". Reporting the first for + the second sends the author hunting for a problem that is not there, and + three of them reworded a correct clause to satisfy a regex instead. + + 'unparsed' is the narrow, certain case: an ADR-0020 arrow clause was + detected and NO target came out of it. The arrow form always names one, so + zero targets means the name is written in a shape the extractor cannot see + — a single-word bare target (`Not X -> forge`, which has to be written + `` `forge` `` or `/forge`) is the live example, since single-word names are + deliberately not matchable bare. + + A PROSE clause yielding no target is NOT reported: "Do not use for anything + else" is a complete and legitimate boundary clause that names nowhere to go. + """ + if BOUNDARY_ARROW.search(description) and not _arrow_targets(description): + return 'unparsed' + if has_boundary_clause(description): + return 'present' + return 'absent' + + +def multi_target_arrow_clauses(description): + """[(first, second)] for arrow clauses naming more than one target. + + Issue #107: only the FIRST target after an arrow is resolved. The + conjunction continuation (CONT_*) is wired to the prose route verbs and + never to arrows, so `Not X -> a or b` resolved `a`, left `b` neither + resolved nor reported, and then printed "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. + + 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; rejecting costs nothing and makes the + one-arrow-per-target convention — already what every retrofitted gitea + skill does in practice — explicit instead of folkloric. The caller emits a + SUGGESTION telling the author to split. + """ + hits = [] + for sentence in SENTENCE_SPLIT.split(description): + matches = (list(ARROW_MARKED.finditer(sentence)) + + list(ARROW_BOUNDARY.finditer(sentence))) + for match in matches: + first, _, _ = _first(match) + if not first: + continue + cont = CONT_ANY.match(sentence, match.end()) + if not cont: + continue + second, _, _ = _first(cont) + if second: + hits.append((first, second)) + return hits + + def unresolved_targets(description, known): """Targets resolving to nothing, split into (blocking, reported). @@ -732,6 +881,17 @@ def unresolved_targets(description, known): Everything else is reported and left alone. `known` is the resolved universe from known_targets(); passing an empty set is not meaningful — callers check for that first and decline out loud instead. + + A NON-TERMINAL target is reported, never dropped. FOLLOWER_OK is a closed + whitelist of maybe eighty words, so the follower rule says "this token is + outside a list I keep" and not "this is prose" — and the old `continue` + turned that into invisibility at every tier. The gate then failed OPEN on + its own unfamiliarity: any target followed by a word nobody thought to + enumerate was neither blocked nor mentioned, so the check that did not run + said nothing about not running. The follower rule may withdraw the power to + BLOCK a commit — that is what it was added for, and the ATTRIBUTIVE USE note + above is the argument for it — but it may not withdraw visibility, which is + the same rule the corroboration tier already follows. """ blocking, reported = set(), set() for sentence in SENTENCE_SPLIT.split(description): @@ -740,7 +900,10 @@ def unresolved_targets(description, known): if normalize_target(name) in known} for name, may_dangle, notation in found: key = normalize_target(name) - if key in known or not may_dangle: + if key in known: + continue + if not may_dangle: + reported.add(name) continue if notation or (resolved - {key}): blocking.add(name) @@ -820,6 +983,47 @@ def description_value(fm_text): return re.sub(r'\s+', ' ', value).strip() +def hand_invoked(fm_text): + """True when the frontmatter marks this file as reached only by hand. + + `disable-model-invocation: true` removes a 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 give such a skill ONE plain human-facing sentence: + no trigger list, no boundary clause. No validator knew the field existed + (issue #108), so the boundary-clause SUGGESTION fired on exactly the shape + the contract mandates, and its remedy — "add a boundary clause 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. + + Only the ROUTING rules are lifted. The body word budget still applies: the + body is loaded on invocation like any other, and competes with the caller's + live conversation the same way. So does the 400-character description FAIL — + a hand-invoked description is not preloaded, but it is still the one line + the user reads when choosing from the `/` menu, and the ceiling is the + outlier stop rather than the style target. + + A parse failure returns False rather than raising. This is a MODIFIER on + other checks, not a check of its own: the frontmatter's validity is decided, + and failed, by description_value() on the same text, and raising a second + exception here would report one broken file twice with two different + diagnoses. + """ + try: + data = yaml.safe_load(fm_text) + except Exception: + return False + if not isinstance(data, dict): + return False + value = data.get('disable-model-invocation') + if isinstance(value, str): + # PyYAML already resolves the unquoted YAML 1.1 booleans, so this only + # catches a QUOTED "true" — which a host reads as truthy and which no + # gate should treat as opting back in to the routing rules. + return value.strip().lower() in ('true', 'yes', 'on') + return value is True + + # --- Body-shape checks (skills only; agents have no references/ dir) ------- # Deterministic and countable, so they are enforced here. Whether a given # gotcha is WARRANTED is semantic and stays the auditor's judgment, which is why @@ -1009,6 +1213,9 @@ for path in files: body = content[fm_match.end():] skill_dir = os.path.dirname(os.path.abspath(path)) + # ADR-0020's hand-invocation carve-out. See hand_invoked() for what it lifts + # and, more importantly, what it does not (issue #108). + by_hand = hand_invoked(fm_match.group(1)) # An absent or empty description is an ERROR here too, not a silent skip. # All three ADR-0020 scripts have to agree on this input: the description is @@ -1030,7 +1237,12 @@ for path in files: "enumeration, output-format detail, composition notes and implementation " "detail to the body or README.md." % (path, dlen, DESC_MAX_CHARS)) - elif dlen > DESC_SUGGEST_CHARS: + elif dlen > DESC_SUGGEST_CHARS and not by_hand: + # The 250-character TARGET is a routing-quality budget: it exists to + # keep the preloaded listing small and the trigger clause sharp. A + # hand-invoked description is in no listing, so there is no budget to + # spend and no shape to enforce. The 400-character FAIL above still + # applies — see hand_invoked(). suggest("%s: description is %d characters, over the %d-character target " "(ADR-0020, hard fail at %d)." % (path, dlen, DESC_SUGGEST_CHARS, DESC_MAX_CHARS)) @@ -1073,15 +1285,40 @@ for path in files: round(100.0 * section_words / body_words), round(100.0 * GOTCHA_MAX_BODY_FRACTION))) - # Missing boundary clause. SUGGESTION, not ERROR: detecting the absence is + # Boundary clause. SUGGESTION, not ERROR: detecting the absence is # deterministic, but whether this particular skill warrants one is the # auditor's call. Both accepted shapes count — the prose markers and # ADR-0020's compressed `Not -> ` arrow. - if desc and not has_boundary_clause(desc): - suggest("%s: description has no boundary clause (ADR-0020). Add the prose form " - "(\"Do not use for X — use `y` instead\") or the compressed form " - "(\"Not X -> y\") so the router knows where NOT to send this skill." - % path) + # + # THREE outcomes, not two. Reporting "no boundary clause" for a clause that + # is present and merely unparsed is a wrong finding, not a strict one, and + # it cost three authors a reworded clause before it was diagnosed (#110). + # + # Skipped entirely for a hand-invoked skill: the contract gives it one plain + # sentence with no boundary clause, so the finding is wrong and its remedy + # names a router that cannot see the skill (#108). + if desc and not by_hand: + status = boundary_clause_status(desc) + if status == 'absent': + suggest("%s: description has no boundary clause (ADR-0020). Add the prose form " + "(\"Do not use for X — use `y` instead\") or the compressed form " + "(\"Not X -> y\") so the router knows where NOT to send this skill." + % path) + elif status == 'unparsed': + suggest("%s: description has an arrow boundary clause (\"Not X -> y\") from which " + "no target could be read, so the dangling-target check did not run on it " + "(ADR-0020). The clause is present — this is a PARSE failure, not a " + "missing clause. Most often the target is a single word, which is " + "deliberately not matchable bare because `research`, `triage` and `forge` " + "are all ordinary English: write it as `name` or /name." % path) + # One arrow, one target. A second name after the arrow is resolved by + # nothing and reported by nothing, so the clause claims coverage it does + # not have (#107). + for first, second in multi_target_arrow_clauses(desc): + suggest("%s: an arrow boundary clause names more than one target ('%s', then " + "'%s'), and only the first is resolved — the second is checked by " + "nothing (ADR-0020). Split it into one arrow per target: " + "\"Not X -> %s. Not Y -> %s.\"" % (path, first, second, first, second)) targets = boundary_targets(desc) if targets: diff --git a/tests/test-adr0020-targets.sh b/tests/test-adr0020-targets.sh index 8fd7d69..3247ee5 100755 --- a/tests/test-adr0020-targets.sh +++ b/tests/test-adr0020-targets.sh @@ -92,8 +92,14 @@ build_tree "$TMPDIR_T/no-claude" build_tree "$TMPDIR_T/with-claude" # The deployed tree, present only in the second root. Both a skill and an agent, # because both are valid routing targets and both would leak. -mkdir -p "$TMPDIR_T/with-claude/.claude/skills/deployed-only-skill" \ - "$TMPDIR_T/with-claude/.claude/agents" +# +# The skill gets a real SKILL.md. That is not decoration: a directory under +# skills/ is a resolvable name only when it HOLDS one, so an empty directory +# would dangle for the wrong reason and the assertion below would pass without +# testing the deployed-tree rule at all. +mkdir -p "$TMPDIR_T/with-claude/.claude/agents" +write_skill "$TMPDIR_T/with-claude/.claude/skills/deployed-only-skill" deployed-only-skill \ + "Use when doing the deployed thing. Do not use for anything else." : > "$TMPDIR_T/with-claude/.claude/agents/deployed-only-agent.md" run_subject() { @@ -133,7 +139,8 @@ fi echo "" echo "--- with no authoring root, a deployed .claude/ tree IS the universe ---" CONSUMER="$TMPDIR_T/consumer" -mkdir -p "$CONSUMER/.claude/skills/deployed-only-skill" +write_skill "$CONSUMER/.claude/skills/deployed-only-skill" deployed-only-skill \ + "Use when doing the deployed thing. Do not use for anything else." write_skill "$CONSUMER/.claude/skills/my-skill" my-skill \ "Use when doing the thing. Do not use for the other thing — use deployed-only-skill instead." set +e @@ -384,8 +391,12 @@ for bait_root in "$BAIT_FRESH" "$BAIT_DEPLOYED"; do write_skill "$bait_root/plugins/bin/.apm/skills/deployed-tree-probe" deployed-tree-probe \ "Use when doing the probe thing. Do not use for the other thing — use $BAIT_NAME instead." done -# Only the deployed copy gets the name planted where `apm install` would put it. -mkdir -p "$BAIT_DEPLOYED/.claude/skills/$BAIT_NAME" "$BAIT_DEPLOYED/.claude/agents" +# Only the deployed copy gets the name planted where `apm install` would put it, +# as a REAL skill directory holding a SKILL.md — an empty directory is not a +# resolvable name, so baiting with one would make the A/B pass vacuously. +mkdir -p "$BAIT_DEPLOYED/.claude/agents" +write_skill "$BAIT_DEPLOYED/.claude/skills/$BAIT_NAME" "$BAIT_NAME" \ + "Use when doing the bait thing. Do not use for anything else." BAIT_FRESH_DANGLING="$(dangling_set "$BAIT_FRESH/plugins")" BAIT_DEPLOYED_DANGLING="$(dangling_set "$BAIT_DEPLOYED/plugins")" @@ -511,13 +522,24 @@ grammar_case() { # The four phrasings that were hard dangling FAILs with no suppression. All four # are lifted from real descriptions in this corpus. -grammar_case fp-precommit-hooks silent "" \ +# +# THEY ARE `suggests`, NOT `silent`, AND THE DIFFERENCE IS THE POINT. The +# follower rule takes away the power to BLOCK a commit on a compound modifier; +# it does not take away visibility, and it used to. FOLLOWER_OK is a closed +# whitelist of about eighty words, so a non-terminal verdict means "the next +# token is outside a list someone maintains by hand", not "this is prose" — and +# `continue`ing on it made the gate fail OPEN on its own unfamiliarity: any +# target followed by an unlisted word was neither blocked nor mentioned at any +# tier. Asserting silence here pinned that hole in place. The assertion that +# still matters is `!= ERROR`, which `suggests` checks, and which is what keeps +# a false positive from stopping a commit. +grammar_case fp-precommit-hooks suggests "routes to 'pre-commit'" \ "Use when running the linter. Use pre-commit hooks instead of ad-hoc scripts." -grammar_case fp-pull-request silent "" \ +grammar_case fp-pull-request suggests "routes to 'pull-request'" \ "Use when opening changes. Invoke the pull-request template instead of writing one by hand." -grammar_case fp-conventional silent "" \ +grammar_case fp-conventional suggests "routes to 'conventional-commits'" \ "Use when writing history. Use conventional-commits formatting rather than free-form messages." -grammar_case fp-prepush-backticked silent "" \ +grammar_case fp-prepush-backticked suggests "routes to 'pre-push'" \ "Use when checking a branch. Do not use for local edits — run the \`pre-push\` hooks instead." echo "" @@ -599,6 +621,165 @@ grammar_case lowercase-start suggests "routes to 'no-such-lower-skill'" \ grammar_case backtick-start suggests "routes to 'no-such-tick-skill'" \ "Use when doing the thing. Use sibling-skill for the main case. \`no-such-tick-skill\` is not for this — do not use it instead." +# --------------------------------------------------------------------------- +# 2a. Route NOTATION always blocks, whatever token follows it +# --------------------------------------------------------------------------- +# FOLLOWER_OK is a closed whitelist of about eighty words. A target followed by +# anything outside it was non-terminal, and `/name` reached _add() with +# strict=None, so it fell to the follower test and lost the power to block — +# contradicting the header's own promise that route notation "always blocks", +# for the one form Claude Code actually uses. Combined with the old `continue` +# in unresolved_targets(), `use /no-such-skill afterwards.` exited 0 with no +# output at all: the gate failed OPEN on a word nobody had thought to enumerate. +# +# "afterwards" is the probe in every case below. It is ordinary English, it is +# not in FOLLOWER_OK, and it is not going to be added to it. +echo "" +echo "--- route notation blocks even when the following token is outside FOLLOWER_OK ---" +grammar_case notation-slash-unlisted errors "routes to 'no-such-slash-skill'" \ + "Use when doing the thing. Do not use for improvements — use /no-such-slash-skill afterwards." +grammar_case notation-arrow-unlisted errors "routes to 'no-such-arrow-skill'" \ + "Use when doing the thing. Not the other thing -> no-such-arrow-skill afterwards." + +echo "" +echo "--- a target the follower rule cannot vouch for is REPORTED, never invisible ---" +# The other half of the same defect, and the one that cost visibility rather +# than enforcement: a PROSE-form target with an unlisted follower may not block +# (that is what the follower rule is for) but it must still be named. Silence +# here is the vacuous-green shape the whole script forbids itself. +grammar_case follower-unlisted-bare suggests "routes to 'no-such-modifier-skill'" \ + "Use when doing the thing. Do not use for improvements — use no-such-modifier-skill afterwards." +grammar_case follower-unlisted-backticked suggests "routes to 'no-such-ticked-skill'" \ + "Use when doing the thing. Do not use for improvements — use \`no-such-ticked-skill\` afterwards." + +# --------------------------------------------------------------------------- +# 2b. Capitalised abbreviations do not over-split a sentence +# --------------------------------------------------------------------------- +# SENTENCE_SPLIT was the one pattern in the resolver built without re.I, so its +# five abbreviation lookbehinds only covered the lowercase spelling. `E.g.` and +# `I.e.` — the SENTENCE-INITIAL spellings, which is exactly where an +# abbreviation lands — matched none of them. The clause split at the +# abbreviation, the corroborating target was stranded on the far side of the +# cut, and a genuinely dangling target silently demoted from blocking ERROR to +# SUGGESTION. The lowercase twin of each case below is `abbrev-split` above and +# already passed, which is precisely why the gap survived. +echo "" +echo "--- a CAPITALISED abbreviation does not strand the corroborator ---" +grammar_case abbrev-split-caps-eg errors "routes to 'no-such-caps-eg-skill'" \ + "Use when doing the thing. Do not use for improvements — use sibling-skill first, E.g. \"run the audit\", then use no-such-caps-eg-skill instead." +grammar_case abbrev-split-caps-ie errors "routes to 'no-such-caps-ie-skill'" \ + "Use when doing the thing. Do not use for improvements — use sibling-skill first, I.e. \"run the audit\", then use no-such-caps-ie-skill instead." + +# --------------------------------------------------------------------------- +# 2c. A boundary clause naming a dotted filename (issue #110) +# --------------------------------------------------------------------------- +# `[^.;]` cannot cross the `.` in `AGENTS.md` or `.pre-commit-config.yaml`, so a +# clause naming a dotted file between "Not" and the arrow was invisible to both +# BOUNDARY_ARROW and ARROW_BOUNDARY. Two different failures came out of that: +# with a backticked target the clause was merely MISDIAGNOSED as missing, and +# with a BARE target it was never extracted at all, so the dangling check +# silently did not run on it. Both directions are pinned. +echo "" +echo "--- a boundary clause naming a dotted filename is seen, and its target is checked ---" +grammar_case dotted-bare-target errors "routes to 'no-such-dotted-skill'" \ + "Use when doing the thing. Not AGENTS.md -> no-such-dotted-skill." +grammar_case dotted-clause-seen silent "" \ + "Use when doing the thing. Not .pre-commit-config.yaml -> sibling-skill." +# The guard that makes the fix a fix and not a hole: a REAL sentence end still +# ends the clause. A `.` followed by whitespace terminates it exactly as before, +# so "Not applicable here." plus an arrow two sentences later is not a boundary +# clause and is still reported as one missing. +grammar_case dotted-sentence-end-guard suggests "has no boundary clause" \ + "Use when doing the thing. Not applicable here. Reproduce -> minimise." + +# --------------------------------------------------------------------------- +# 2d. "Present but unparsed" is a different finding from "missing" +# --------------------------------------------------------------------------- +# Issue #110's standing request. An arrow clause ALWAYS names a target, so one +# that yields none is a parse failure and must say so — telling the author the +# clause is missing sends them to add a second copy of a clause that is already +# there. The live shape is a single-word target, which is deliberately not +# matchable bare because `research`, `triage` and `forge` are all skill names +# AND ordinary English. +echo "" +echo "--- an arrow clause that yields no target is reported as unparsed, not as missing ---" +grammar_case arrow-single-word-target suggests "no target could be read" \ + "Use when doing the thing. Not the other thing -> forge." +# Control, so the case above is not satisfied by a check that fires on every +# arrow clause: the same clause with the target written in a shape the extractor +# can see produces nothing at all. +grammar_case arrow-single-word-marked silent "" \ + "Use when doing the thing. Not the other thing -> \`sibling-skill\`." + +# --------------------------------------------------------------------------- +# 2e. One arrow, one target (issue #107) +# --------------------------------------------------------------------------- +# Only the first target after an arrow is resolved: the conjunction continuation +# is wired to the prose route verbs and never to arrows. So the second name in +# `Not X -> a or b` was resolved by nothing and reported by nothing, and the +# audit then printed "1 of 1 boundary target(s) resolve" on a clause naming two. +# A typo in the second target shipped through a green gate. +# +# The fix rejects the shape rather than widening the extractor. The case below +# is the exact failure: a bare `Not ... ->` sentence carries no BOUNDARY_MARKER, +# so the backtick sweep does not run and the second target is genuinely +# invisible to every other rule in the resolver. +echo "" +echo "--- an arrow clause naming two targets is rejected, so the unchecked one is visible ---" +grammar_case multi-arrow-second-target suggests "names more than one target" \ + "Use when doing the thing. Not the other thing -> \`sibling-skill\` or \`no-such-second-target\`." +grammar_case multi-arrow-comma suggests "names more than one target" \ + "Use when doing the thing. Not the other thing -> \`sibling-skill\`, \`no-such-comma-target\`." +# Control: one arrow, one target — the convention the SUGGESTION is asking for — +# stays silent. Without this the case above is satisfied by a check that fires +# on every arrow clause in the corpus. +grammar_case multi-arrow-control silent "" \ + "Use when doing the thing. Not the other thing -> \`sibling-skill\`." + +# --------------------------------------------------------------------------- +# 2f. A skill directory with no SKILL.md is not a skill +# --------------------------------------------------------------------------- +# _collect_package() added a name for every directory matching skills/*/, with +# no check that anything was in it. A leftover empty directory — a deleted skill +# whose directory survived, a scaffolding stub, an editor's stray mkdir — is +# untracked by git, so it exists on the machine that made it and nowhere else. +# The hook went green locally and red in a fresh clone: the same +# install-dependence the deployed-tree rule exists to remove, arriving through a +# different door. Both directions are asserted, because "never resolve" would +# also satisfy the first half. +echo "" +echo "--- an empty skills// directory does not make a routing target resolve ---" +GHOST="$TMPDIR_T/ghost-dir" +write_skill "$GHOST/plugins/p/.apm/skills/my-skill" my-skill \ + "Use when doing the thing. Do not use for the other thing — use /ghost-skill instead." +mkdir -p "$GHOST/plugins/p/.apm/skills/ghost-skill" +# NOT wrapped in a helper function: command substitution runs the body in a +# subshell, so an exit status assigned inside one never reaches the caller — +# under `set -u` the second read of it aborts the suite. +set +e +GHOST_OUT="$(bash "$HOOK" "$GHOST/plugins/p/.apm/skills/my-skill/SKILL.md" 2>&1)" +GHOST_RC=$? +set -e +if [[ $GHOST_RC -ne 0 && "$GHOST_OUT" == *"routes to 'ghost-skill'"* ]]; then + pass "a directory with no SKILL.md in it is not a resolvable name" +else + fail "an empty skills/ghost-skill/ directory resolved a routing target (exit $GHOST_RC): ${GHOST_OUT:-}" +fi +# The confirming half: drop a SKILL.md into the same directory and the identical +# description resolves. Without this the rule could be implemented as "skills/ +# never contributes anything" and still pass above. +write_skill "$GHOST/plugins/p/.apm/skills/ghost-skill" ghost-skill \ + "Use when doing the other thing. Do not use for anything else." +set +e +GHOST_OUT="$(bash "$HOOK" "$GHOST/plugins/p/.apm/skills/my-skill/SKILL.md" 2>&1)" +GHOST_RC=$? +set -e +if [[ $GHOST_RC -eq 0 && -z "$GHOST_OUT" ]]; then + pass "the same directory WITH a SKILL.md resolves, so the rule is 'no SKILL.md' and not 'never'" +else + fail "a populated skills/ghost-skill/ directory still did not resolve (exit $GHOST_RC): ${GHOST_OUT:-}" +fi + # And the confirming half of the grammar rule: a compound-modifier target is # CONFIRM-ONLY, not ignored. When the name does exist it still counts as a route # — the rule suppresses the ERROR, it does not delete the target. diff --git a/tests/test-skill-size-check.sh b/tests/test-skill-size-check.sh index 3423850..2762e8b 100755 --- a/tests/test-skill-size-check.sh +++ b/tests/test-skill-size-check.sh @@ -331,13 +331,32 @@ PY # # The sibling plugin is what makes "every plugin in the monorepo contributes its # names" testable; without it a cross-plugin target and a typo are the same. +# +# Both sibling skill directories get a real SKILL.md, and that is load-bearing +# rather than tidiness: a skill directory is a resolvable name only if it HOLDS +# a SKILL.md. An empty leftover directory is untracked by git, so counting one +# made a target resolve on the machine that made it and dangle in a fresh clone +# — the same install-dependence the deployed-tree rule exists to remove. This +# fixture used to `mkdir` the two siblings and write nothing into them, so it +# was itself relying on the behaviour the resolver no longer has. make_tree_fixture() { - local label="$1" desc="$2" body_words="$3" root apm + local label="$1" desc="$2" body_words="$3" root apm sib root="$TMPDIR/tree-$label" apm="$root/plugins/subject-plugin/.apm" mkdir -p "$apm/skills/$label" "$apm/skills/sibling-skill" "$apm/agents" \ "$root/plugins/other-plugin/.apm/skills/cross-plugin-skill" : > "$apm/agents/sibling-agent.agent.md" + for sib in "$apm/skills/sibling-skill" \ + "$root/plugins/other-plugin/.apm/skills/cross-plugin-skill"; do + { + echo "---" + echo "name: $(basename "$sib")" + echo "description: Use when doing the other thing. Do not use for anything else." + echo "---" + echo "" + echo "Do the thing." + } > "$sib/SKILL.md" + done { echo "---" echo "name: $label" @@ -364,8 +383,17 @@ expect_gate() { fail "$label (exit $status, output: ${out:-})" fi ;; + # The tier and the needle are matched ADJACENTLY — `*"SUGGESTION"*"$needle"*` + # — not as two independent substring tests. Independently, any output + # carrying a SUGGESTION anywhere and the needle anywhere satisfied the + # assertion, so a needle emitted at the WRONG TIER still passed: a finding + # that moved from SUGGESTION to a blocking ERROR line would be caught only + # by the exit-status test, and one that moved from SUGGESTION to INFO would + # not be caught at all. grammar_case's `suggests` branch in + # tests/test-adr0020-targets.sh has always matched them adjacently; this is + # the same rule. suggest) - if [[ $status -eq 0 && "$out" == *"SUGGESTION"* && "$out" == *"$needle"* ]]; then + if [[ $status -eq 0 && "$out" == *"SUGGESTION"*"$needle"* ]]; then pass "$label" else fail "$label (exit $status, output: ${out:-})" @@ -450,9 +478,14 @@ expect_gate "body at $((BODY_MAX_WORDS + 1)) words fails" \ echo "" echo "--- the body gate and the whole-file gate are independent measurements ---" BODY_ONLY_DESC="$(python3 -c "print(' '.join(['w'] * 100))")" +# The needle pins the COUNT, not the bare word "words". "words" appears in the +# whole-file ceiling message, in the body ceiling message and in the body target +# message alike, so it was satisfied by any of the three — including the one +# this case exists to prove does NOT fire. Naming the number is what makes the +# assertion about the body-only measurement. expect_gate "frontmatter words do not count toward the $BODY_MAX_WORDS-word body ceiling" \ suggest "$(make_budget_fixture body-independent "$BODY_ONLY_DESC" "$((BODY_MAX_WORDS - 5))")" \ - "words" + "body is $((BODY_MAX_WORDS - 5)) words" BIG_BODY="$(make_budget_fixture body-over-not-whole-file "$CLEAN_DESC" "$((BODY_MAX_WORDS + 1))")" BIG_BODY_WORDS="$(wc -w < "$BIG_BODY")" if [[ "$BIG_BODY_WORDS" -le "$MAX_WORDS" ]]; then @@ -461,6 +494,96 @@ else fail "the body-gate fixture is $BIG_BODY_WORDS whole-file words, which also trips MAX_WORDS=$MAX_WORDS — the test no longer isolates the body gate" fi +# --------------------------------------------------------------------------- +# ADR-0020's hand-invocation carve-out (issue #108) +# --------------------------------------------------------------------------- +# A skill carrying `disable-model-invocation: true` is removed 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, skill-author Step 2 and skill-audit's own Step 0 all give it ONE +# plain human-facing sentence: no trigger list, no boundary clause. No validator +# knew the field existed, 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" — named a router that cannot see the skill at all. +# +# The carve-out is NARROW and the half it does not cover is the half worth +# testing: the body is still loaded on invocation, so the body budget stands, +# and the 400-character ceiling stands because it is an outlier stop rather than +# a routing-quality target. Every case below asserts one of those two halves. +make_hand_invoked_fixture() { + local name="$1" desc="$2" body_words="$3" file + file="$TMPDIR/$name.md" + { + echo "---" + echo "name: $name" + echo "description: $desc" + echo "disable-model-invocation: true" + echo "---" + echo "" + python3 -c "print(' '.join(['word'] * $body_words))" + } > "$file" + echo "$file" +} + +# A description over the 250-character target, carrying no boundary clause and +# no routing target — the exact shape `zoom-out` and `caveman` ship. Built with +# no hyphens so nothing in it reads as a target. +HAND_DESC="$(python3 -c " +prefix = 'Tell the agent to zoom out and give broader context. ' +print(prefix + 'x' * (300 - len(prefix)))")" + +echo "" +echo "--- a hand-invoked skill is exempt from the routing rules, and only those ---" +expect_gate "a hand-invoked skill with a 300-char description and no boundary clause is silent" \ + pass "$(make_hand_invoked_fixture hand-quiet "$HAND_DESC" 10)" +# The control that makes the case above mean something. Same description, same +# body, only the frontmatter flag removed: both findings must appear, or the +# exemption is being credited for silence it did not cause. +expect_gate "control: the SAME description without the flag is over the 250-char target" \ + suggest "$(make_budget_fixture hand-control "$HAND_DESC" 10)" \ + "description is 300 characters" +expect_gate "control: the SAME description without the flag has no boundary clause" \ + suggest "$(make_budget_fixture hand-control "$HAND_DESC" 10)" \ + "has no boundary clause" + +echo "" +echo "--- the carve-out lifts the routing rules ONLY: both size gates still bite ---" +# The description ceiling is not a routing budget: a hand-invoked description is +# still the one line a human reads in the `/` menu, and 400 characters is the +# outlier stop either way. +HAND_OVER_MAX="$(python3 -c " +prefix = 'Tell the agent to zoom out and give broader context. ' +print(prefix + 'x' * (401 - len(prefix)))")" +expect_gate "a hand-invoked description over $DESC_MAX_CHARS chars still FAILS" \ + fail "$(make_hand_invoked_fixture hand-over-max "$HAND_OVER_MAX" 10)" \ + "$DESC_MAX_CHARS-character ceiling" +# The body is loaded on invocation like any other body and competes with the +# caller's live conversation exactly the same way, so neither body tier moves. +expect_gate "a hand-invoked body over $BODY_MAX_WORDS words still FAILS" \ + fail "$(make_hand_invoked_fixture hand-over-body "$HAND_DESC" "$((BODY_MAX_WORDS + 1))")" \ + "$BODY_MAX_WORDS-word ceiling" +expect_gate "a hand-invoked body over $BODY_SUGGEST_WORDS words is still suggested" \ + suggest "$(make_hand_invoked_fixture hand-over-body-suggest "$HAND_DESC" "$((BODY_SUGGEST_WORDS + 1))")" \ + "body is $((BODY_SUGGEST_WORDS + 1)) words" + +echo "" +echo "--- the flag is read as a BOOLEAN, not as any mention of the key ---" +# `disable-model-invocation: false` is the model-invoked case written out +# longhand. Reading the key's presence instead of its value would hand every +# routing exemption to anyone who typed the field at all. +HAND_FALSE="$TMPDIR/hand-false.md" +{ + echo "---" + echo "name: hand-false" + echo "description: $HAND_DESC" + echo "disable-model-invocation: false" + echo "---" + echo "" + echo "Do the thing." +} > "$HAND_FALSE" +expect_gate "disable-model-invocation: false is NOT the carve-out" \ + suggest "$HAND_FALSE" "has no boundary clause" + echo "" echo "--- resolvable boundary targets ---" # Resolution is against the AUTHORING SOURCE (plugins/*/.apm/skills/ and