fix(gates): check body-level routing targets, not just descriptions
The ADR-0020 boundary resolver (boundary_targets()/unresolved_targets()) only ever read a SKILL.md's description. A target named in the BODY -- a dispatch table row, a "run X" step, both routine in a 900-word procedure -- was checked by nothing. Two real instances shipped before either was caught by reading rather than by a gate: bin/write-docs routed twice to a deleted `to-prd` skill, and bin/triage told an agent to run a nonexistent `/setup-matt-pocock-skills` (both fixed in 03abcff; that fix was the symptom, this gate is the actual ask per #124). Added a separate, narrower extractor -- body_targets() / unresolved_body_targets() in the shared lib-boundary-resolver.sh -- rather than reusing the description resolver at wider scope. The description gate's sentence-level heuristics (BOUNDARY_MARKER, the follower test, in-sentence corroboration) are tuned for a one-to-three-sentence routing clause and misfire on dispatch-table/procedure prose in both directions, so the body gate reads only explicit route notation (`/name`, backticked-or-slash-prefixed `-> name` / `-> name`), already the description gate's own unconditionally-blocking tier. Three guards were added after running the extractor over the real 39-skill corpus and reading every hit rather than assuming the design was correct: - a target must be hyphenated, even in notation -- single-word citations like `/fork` (forge, citing Claude Code's own /fork command) and `/name` (skill-author, a placeholder) are not routes. - a bare hyphenated word after any arrow is not notation -- only ARROW_MARKED (backticked/slash-prefixed) is used, not NOTATION_ARROW's bare form, so ordinary process-chain prose ("prop -> new ref -> re-render", caveman) is not read as a route. - a name immediately preceded by `<` is a closing tag (`</what-to-do>`, grill-with-docs), not /name notation. Wired into both consumers that must agree by contract: scripts/ skill-size-check.sh (the pre-commit hook) and factory-audit's lib-checks-skill.sh (the audit). Verified identical findings across both over the whole corpus. tests/test-adr0020-targets.sh gains a dedicated section pinning the two live true positives and all three guards. docs/spec/gates.md and ADR-0020 get a matching amendment. Fixes: #124 ADR: 0020 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGHFJextYtVQseaHPDDhxB
This commit is contained in:
@@ -7,7 +7,7 @@ description: >
|
||||
fixes -> agent-author.
|
||||
allowed-tools: Bash Read
|
||||
metadata:
|
||||
version: "1.0.4"
|
||||
version: "1.0.5"
|
||||
category: factory
|
||||
source_keys:
|
||||
- agentskills-home
|
||||
|
||||
@@ -898,6 +898,103 @@ def unresolved_targets(description, known):
|
||||
reported.add(name)
|
||||
return sorted(blocking), sorted(reported - blocking)
|
||||
|
||||
|
||||
# --- Body-level routing targets (issue #124) -------------------------------
|
||||
# boundary_targets()/unresolved_targets() above are tuned for a description:
|
||||
# one to three sentences, where BOUNDARY_MARKER, the follower test and
|
||||
# in-sentence corroboration all exist to tell a routing sentence apart from
|
||||
# ordinary prose about a hyphenated tool. A SKILL.md body is a different
|
||||
# genre — up to 900 words of procedure and dispatch tables — where those same
|
||||
# heuristics would misfire in both directions: a dispatch table rarely reads
|
||||
# as a "boundary sentence" (under-fire), and a procedure step naming a file, a
|
||||
# CLI verb or a config key looks exactly like a route (over-fire). Retuning
|
||||
# the sentence-level heuristics for that genre is the hard half of this gate
|
||||
# and is deliberately NOT attempted here — see the issue for why.
|
||||
#
|
||||
# So the body extractor takes the narrow route instead: only two EXPLICIT
|
||||
# ROUTE NOTATION forms count, and each is measured against the real corpus
|
||||
# (39 SKILL.md bodies) rather than assumed correct from the description gate's
|
||||
# behaviour — a body is dense with prose that LOOKS like this notation and
|
||||
# genuinely is not, in ways a one-to-three-sentence description never is:
|
||||
#
|
||||
# * ARROW_MARKED — `-> name` / `→ name` where the target is BACKTICKED or
|
||||
# slash-prefixed (MARKED_TARGET). NOT NOTATION_ARROW, which matches a bare
|
||||
# hyphenated word after any arrow: the corpus's own process-chain prose
|
||||
# ("Inline obj prop -> new ref -> re-render.", caveman/SKILL.md) reads as
|
||||
# a route under that pattern and does not under this one, because a
|
||||
# process chain is never itself backticked or slash-prefixed. The one
|
||||
# live true positive this was filed over, write-docs' "-> `to-prd`", IS
|
||||
# backticked (03abcff's diff shows the original), so ARROW_MARKED still
|
||||
# catches it losslessly.
|
||||
# * NOTATION_SLASH — free-standing `/name`, unconditionally, the same
|
||||
# pattern the description gate sweeps with. Two guards narrow it for body
|
||||
# text specifically, each one measured against a real corpus false
|
||||
# positive rather than hypothesised:
|
||||
# - a name with NO hyphen is discarded. A real dispatch entry in this
|
||||
# corpus always names a multi-word skill (`to-prd`,
|
||||
# `setup-matt-pocock-skills`); a single bare or backticked word after
|
||||
# a `/` is prose citing a CLI command, a Claude Code built-in or a
|
||||
# placeholder — `` `/fork` `` (forge/SKILL.md, contrasting
|
||||
# `context: fork` with Claude Code's own /fork subagent command) and
|
||||
# `` `/name` `` (skill-author/SKILL.md, "the user types `/name`" —
|
||||
# `name` is a placeholder for the skill's OWN name, not a route) are
|
||||
# both real corpus hits this guard removes. This is a real recall
|
||||
# loss — `/forge`, `/triage` and other single-word skill names are
|
||||
# unreachable through this extractor — accepted deliberately, the
|
||||
# same "start narrow" trade the issue itself recommends.
|
||||
# - a name immediately preceded by `<` is discarded. An XML/HTML-style
|
||||
# closing tag used as a prompt section delimiter — `</what-to-do>`,
|
||||
# `</supporting-info>` (grill-with-docs/SKILL.md) — is indistinguishable
|
||||
# from `/what-to-do` notation by every other rule in this pattern; no
|
||||
# route is ever written directly after `<` in this corpus, so the
|
||||
# guard costs nothing else.
|
||||
#
|
||||
# Every surviving hit is unconditionally blocking: both forms are explicit
|
||||
# notation with the ambiguous single-word and closing-tag readings already
|
||||
# removed, so there is no SUGGESTION tier here — that tier exists to soften
|
||||
# an ambiguous prose form, and none is admitted at this point.
|
||||
#
|
||||
# No conjunction continuation (CONT_*) either: `-> \`to-prd\` or \`grill-me\``
|
||||
# resolves only `to-prd`, the same one-arrow-one-target convention
|
||||
# multi_target_arrow_clauses() already enforces on descriptions (issue #107),
|
||||
# applied here by construction instead of by a second SUGGESTION.
|
||||
def body_targets(body):
|
||||
"""Every /name or -> `name` routing target named in a SKILL.md body.
|
||||
|
||||
Fenced code blocks are masked first, the same way gotcha_stats() and
|
||||
missing_reference_pointers() mask them: a ```-fenced example quoting
|
||||
`/some-skill` or `-> \`some-skill\`` as illustration is not a live
|
||||
dispatch entry, and skill-author/factory-audit — which document this
|
||||
very notation — are exactly the skills most likely to carry one.
|
||||
"""
|
||||
masked = mask_fenced(body)
|
||||
names = set()
|
||||
for match in NOTATION_SLASH.finditer(masked):
|
||||
if match.start() > 0 and masked[match.start() - 1] == '<':
|
||||
continue # </closing-tag>, not /route-notation
|
||||
name = match.group(1)
|
||||
if '-' in name:
|
||||
names.add(name)
|
||||
for match in ARROW_MARKED.finditer(masked):
|
||||
name, _, _ = _first(match)
|
||||
if name and '-' in name:
|
||||
names.add(name)
|
||||
return sorted(names)
|
||||
|
||||
|
||||
def unresolved_body_targets(body, known):
|
||||
"""Body routing targets (notation only) that resolve to nothing.
|
||||
|
||||
Unlike unresolved_targets(), this has one outcome, not two: every name
|
||||
body_targets() finds is already route notation, and notation always
|
||||
blocks. `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, exactly as they do for the description gate.
|
||||
"""
|
||||
return sorted(name for name in body_targets(body)
|
||||
if normalize_target(name) not in known)
|
||||
|
||||
|
||||
# --- Frontmatter ----------------------------------------------------------
|
||||
# Tolerant on the way in, HARD-FAILING on the way out. A UTF-8 BOM, a leading
|
||||
# blank line, trailing whitespace after either `---`, or CRLF line endings all
|
||||
|
||||
@@ -443,39 +443,56 @@ elif desc:
|
||||
# derived from this script's own path, and — when an authoring root exists — it
|
||||
# never reads a deployed .claude/ tree, so a fresh clone and a machine that has
|
||||
# run `apm install` return the same verdict. See the shared resolver's header.
|
||||
if desc:
|
||||
routing_targets = boundary_targets(desc)
|
||||
known = known_targets(skill_dir) if routing_targets else set()
|
||||
if routing_targets and not known:
|
||||
routing_targets = boundary_targets(desc) if desc else []
|
||||
# Body-level targets (issue #124): notation only (`/name`, `-> name`), so
|
||||
# every hit is unconditionally blocking — see the shared resolver's
|
||||
# body_targets() header for why the description gate's SUGGESTION tier has
|
||||
# no counterpart here. Read regardless of `desc`: a body dispatch table can
|
||||
# carry a broken route even when the description carries none.
|
||||
body_routing_targets = body_targets(body)
|
||||
if routing_targets or body_routing_targets:
|
||||
known = known_targets(skill_dir)
|
||||
if not known:
|
||||
unchecked = sorted(set(routing_targets) | set(body_routing_targets))
|
||||
info(f"boundary-target resolution DID NOT RUN — no skill universe could be "
|
||||
f"determined for this path (no authoring root above it, no apm package "
|
||||
f"root, no declared apm dependencies, no deployed .claude/ or .agents/ "
|
||||
f"tree). Unchecked target(s): {', '.join(routing_targets)}")
|
||||
elif routing_targets:
|
||||
# blocking vs reported: a target only earns a FAIL when it is written in
|
||||
# route notation or its own sentence corroborates it by naming another
|
||||
# target that resolves. See the shared resolver's CORROBORATION note.
|
||||
unresolved, soft = unresolved_targets(desc, known)
|
||||
for target in unresolved:
|
||||
fail(f"description routes to '{target}', which resolves to no skill or agent "
|
||||
f"in this monorepo, in this package, or in a package it declares in "
|
||||
f"apm.yml dependencies.apm — a boundary clause naming a non-existent "
|
||||
f"target sends the router nowhere")
|
||||
for target in soft:
|
||||
suggest(f"description routes to '{target}', which resolves to no skill or agent "
|
||||
f"in this monorepo, in this package, or in a package it declares in "
|
||||
f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing "
|
||||
f"else in that sentence resolves, so it is equally likely to be a tool, a "
|
||||
f"file format or an English compound. If it IS a route, write it as "
|
||||
f"`/{target}` or `-> {target}` and it will be checked properly")
|
||||
if not unresolved:
|
||||
# Counts the targets that ACTUALLY resolve, not every target found:
|
||||
# a confirm-only target (one used attributively — see the resolver's
|
||||
# ATTRIBUTIVE USE note) is exempt from the failure above, so
|
||||
# reporting it as resolved would be a false claim.
|
||||
resolved = [t for t in routing_targets if normalize_target(t) in known]
|
||||
ok(f"{len(resolved)} of {len(routing_targets)} boundary target(s) resolve: "
|
||||
f"{', '.join(resolved) if resolved else '(none)'}")
|
||||
f"tree). Unchecked target(s): {', '.join(unchecked)}")
|
||||
else:
|
||||
if routing_targets:
|
||||
# blocking vs reported: a target only earns a FAIL when it is written in
|
||||
# route notation or its own sentence corroborates it by naming another
|
||||
# target that resolves. See the shared resolver's CORROBORATION note.
|
||||
unresolved, soft = unresolved_targets(desc, known)
|
||||
for target in unresolved:
|
||||
fail(f"description routes to '{target}', which resolves to no skill or agent "
|
||||
f"in this monorepo, in this package, or in a package it declares in "
|
||||
f"apm.yml dependencies.apm — a boundary clause naming a non-existent "
|
||||
f"target sends the router nowhere")
|
||||
for target in soft:
|
||||
suggest(f"description routes to '{target}', which resolves to no skill or agent "
|
||||
f"in this monorepo, in this package, or in a package it declares in "
|
||||
f"apm.yml dependencies.apm — SUGGESTION rather than FAIL because nothing "
|
||||
f"else in that sentence resolves, so it is equally likely to be a tool, a "
|
||||
f"file format or an English compound. If it IS a route, write it as "
|
||||
f"`/{target}` or `-> {target}` and it will be checked properly")
|
||||
if not unresolved:
|
||||
# Counts the targets that ACTUALLY resolve, not every target found:
|
||||
# a confirm-only target (one used attributively — see the resolver's
|
||||
# ATTRIBUTIVE USE note) is exempt from the failure above, so
|
||||
# reporting it as resolved would be a false claim.
|
||||
resolved = [t for t in routing_targets if normalize_target(t) in known]
|
||||
ok(f"{len(resolved)} of {len(routing_targets)} boundary target(s) resolve: "
|
||||
f"{', '.join(resolved) if resolved else '(none)'}")
|
||||
unresolved_body = unresolved_body_targets(body, known)
|
||||
for target in unresolved_body:
|
||||
fail(f"body routes to '{target}' (`/{target}` or `-> {target}` notation), which "
|
||||
f"resolves to no skill or agent in this monorepo, in this package, or in a "
|
||||
f"package it declares in apm.yml dependencies.apm — a dispatch table or "
|
||||
f"\"run X\" step naming a non-existent target sends the agent nowhere")
|
||||
if body_routing_targets and not unresolved_body:
|
||||
ok(f"{len(body_routing_targets)} of {len(body_routing_targets)} body routing "
|
||||
f"target(s) resolve: {', '.join(body_routing_targets)}")
|
||||
|
||||
# Body unfilled placeholders
|
||||
fill_matches = PLACEHOLDER_RE.findall(body)
|
||||
|
||||
Reference in New Issue
Block a user