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.
This commit is contained in:
@@ -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 <thing> -> <name>`.
|
||||
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'(?<!\\be\\.g\\.)(?<!\\bi\\.e\\.)(?<!\\betc\\.)(?<!\\bvs\\.)(?<!\\bcf\\.)'
|
||||
u'(?<=[.!?])\\s+(?=[A-Za-z`"“(])')
|
||||
u'(?<=[.!?])\\s+(?=[A-Za-z`"“(])', re.I)
|
||||
|
||||
# The token that may follow a route target without turning it into a compound
|
||||
# modifier: punctuation, end of sentence, a conjunction, a boundary word, or a
|
||||
@@ -643,11 +680,26 @@ def _notation(text, start, arrow):
|
||||
|
||||
|
||||
def _add(out, text, name, start, end, strict=None, arrow=False):
|
||||
"""Record one target as (name, may_dangle, notation).
|
||||
|
||||
NOTATION IS DECIDED FIRST, and when it is set the follower test is skipped.
|
||||
The header above promises that route notation "always blocks", and for the
|
||||
`/name` form that was false: `-> 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 <thing> -> <name>`. 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():
|
||||
|
||||
Reference in New Issue
Block a user