fix(skill-audit): make check 9 reachable, wrap-safe and never silently skipped
Check 9 shipped in #130 to close #118, but three defects meant it could not do the job it was added for. Why: - It is INFO-only, so it always exits 0 — and SKILL.md graded exit 0 "a genuine pass" and said the script "prints nothing on success". Every check-9 INFO was discarded before it reached a report, behind three further doors that only opened on a non-zero exit. - `parse_field_raw()` matched `(.+)`, which does not span newlines, so only the first physical line of a wrapped value was compared. Rewriting only the continuation line of a wrapped Description from a hedge to a confident claim produced no finding at all — verbatim the regression #118 was filed about. The bullet branch had the same shape: a wrapped bullet broke the loop and dropped every later entry. - A `git show` failure at the base ref was treated as "creation, nothing to flag" and skipped the whole skill with no output, collapsing "absent at that ref" with "not tracked under that name". A gitignored `.claude/skills/` copy reported clean while the authoring path reported four changed claims. The script's own usage text promises this is "never a silent skip". Implementation notes: - Exit-code guidance re-keyed on output as well as code: 0-and-silent passes, 0-with-output is INFO-only findings, 1 is FAILs, 2 never ran. - `parse_field_raw()` is line-based and joins continuation lines; `normalize_field_text()`'s docstring is now true rather than aspirational. A reorder deliberately fires: the two fields share one parser, and order-insensitivity would mean splitting a prose Description on commas. - The discarded `show_err` is now surfaced as one whole-check INFO naming both readings. - `--base-ref=` given empty now beats the env var, as the usage text always claimed. `validate.sh` gains an ADR-0022 `metadata.version` check at FAIL tier, because any lower tier lets skill-author Step 4 report done on a file the commit gate then refuses. Its `read` heuristic now skips here-doc bodies — reflowing the one offending line would have cleared the finding and left the cause, since every usage() heredoc is one wrap from putting the English verb in column 0. Impact: provenance tests 73 -> 82, validate tests 64 -> 72. Test 72 previously deleted origin/main before asserting the override, so it proved the flag works with no default rather than that it beats one; it now moves origin/main forward first. Refs: #118 ADR: 0022 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeH8SCbcrCAQrtymkNuhKP
This commit is contained in:
@@ -1289,6 +1289,50 @@ else:
|
||||
if desc:
|
||||
ok("description has no unfilled placeholders")
|
||||
|
||||
# --- ADR-0022: metadata.version is mandatory -------------------------------
|
||||
# FAIL, not SUGGESTION, and the tier is set by the gate rather than by taste.
|
||||
# `.pre-commit-config.yaml`'s `skill-frontmatter` hook REJECTS a SKILL.md with
|
||||
# no `metadata.version`, and rejects a value that is not three-part semver.
|
||||
# skill-author's Step 4 says to run this audit and "resolve every FAIL", so any
|
||||
# tier below FAIL lets that step report done on a skill the commit gate then
|
||||
# refuses — the same audit-disagrees-with-the-gate failure the MAX_LINES note
|
||||
# below warns about, arrived at from the other direction. Verified before this
|
||||
# check existed: a SKILL.md with no `metadata:` block at all reported "All
|
||||
# checks passed".
|
||||
#
|
||||
# The rule is DUPLICATED from that hook for the same cache-isolation reason as
|
||||
# every other constant here — an installed plugin's scripts cannot read the
|
||||
# repo-root config. Keep the two in step: this check must accept exactly what
|
||||
# the hook accepts.
|
||||
SEMVER_RE = re.compile(r'^\d+\.\d+\.\d+$')
|
||||
|
||||
try:
|
||||
fm_data = yaml.safe_load(fm)
|
||||
except Exception:
|
||||
# Unreachable in practice: description_value() above parses the same text
|
||||
# and hard-exits on a YAML error, so anything arriving here already parsed.
|
||||
fm_data = None
|
||||
metadata_block = fm_data.get('metadata') if isinstance(fm_data, dict) else None
|
||||
|
||||
if not isinstance(metadata_block, dict) or metadata_block.get('version') is None:
|
||||
fail("frontmatter has no metadata.version — ADR-0022 makes it mandatory for "
|
||||
"every skill, and the skill-frontmatter pre-commit hook rejects the file "
|
||||
"without it. Add `metadata:` / ` version: \"1.0.0\"` (new skills start "
|
||||
"at \"0.1.0\")")
|
||||
else:
|
||||
version_value = metadata_block['version']
|
||||
# NOT str()-coerced blind: `version: 1.0` is a YAML float, and its "1.0"
|
||||
# spelling is exactly the two-part value the hook rejects — coercing and
|
||||
# then matching keeps this check and the hook agreeing on that case.
|
||||
version_text = version_value if isinstance(version_value, str) else str(version_value)
|
||||
version_text = version_text.strip()
|
||||
if SEMVER_RE.match(version_text):
|
||||
ok(f"metadata.version present: '{version_text}' (ADR-0022)")
|
||||
else:
|
||||
fail(f"metadata.version '{version_text}' is not three-part semver — the "
|
||||
f"skill-frontmatter pre-commit hook rejects it. Use MAJOR.MINOR.PATCH, "
|
||||
f"e.g. \"1.0.0\"")
|
||||
|
||||
# SKILL.md size ceilings (agentskills.io skill-authoring.md: 500 lines,
|
||||
# ~5,000 tokens). Both constants are DUPLICATED from the repo-root pre-commit
|
||||
# hook scripts/skill-size-check.sh — a plugin skill's scripts cannot read files
|
||||
@@ -1515,11 +1559,66 @@ def stdin_redirected(line, prev_line):
|
||||
unquoted = re.sub(r'"[^"]*"|\'[^\']*\'', '', line)
|
||||
return '<' in unquoted or prev_line.rstrip().endswith('|')
|
||||
|
||||
# A here-doc body is DATA, not command position. Every script in this corpus
|
||||
# carries a `usage() { cat <<EOF ... EOF; }`, and prose wrapped inside one puts
|
||||
# ordinary English at the start of a line — "read is reported as an INFO ..."
|
||||
# in this skill's own validate-provenance.sh, which made skill-audit hard-FAIL
|
||||
# on its own script. Reflowing that one sentence would have cleared the finding
|
||||
# and left the cause: every future usage text is one wrap away from the same
|
||||
# false positive, and the remedy an author reaches for is contorting working
|
||||
# source, which the note above records has already happened twice.
|
||||
#
|
||||
# Detection is deliberately conservative in the direction that matters. A
|
||||
# here-doc body is skipped only when its terminator is actually found further
|
||||
# down the file; an opener with no terminator — the shape a stray `<<` inside a
|
||||
# string would produce — is ignored rather than allowed to swallow the tail,
|
||||
# because swallowing the tail is a false NEGATIVE and this check exists to fail
|
||||
# closed. `<<<` here-strings open nothing and are excluded by the lookbehind.
|
||||
HEREDOC_START_RE = re.compile(r'(?<!<)<<-?\s*(["\']?)([A-Za-z_][A-Za-z0-9_]*)\1')
|
||||
|
||||
|
||||
def heredoc_delimiter(line):
|
||||
"""The here-doc terminator this line opens, or None."""
|
||||
m = HEREDOC_START_RE.search(line)
|
||||
return m.group(2) if m else None
|
||||
|
||||
|
||||
def heredoc_body_indices(lines):
|
||||
"""Line indices that are here-doc BODY (plus its terminator), not code."""
|
||||
skip = set()
|
||||
i, n = 0, len(lines)
|
||||
while i < n:
|
||||
stripped = lines[i].strip()
|
||||
delim = None if stripped.startswith('#') else heredoc_delimiter(lines[i])
|
||||
if delim:
|
||||
# `<<-` allows an indented terminator, so compare stripped.
|
||||
for j in range(i + 1, n):
|
||||
if lines[j].strip() == delim:
|
||||
skip.update(range(i + 1, j + 1))
|
||||
i = j
|
||||
break
|
||||
i += 1
|
||||
return skip
|
||||
|
||||
|
||||
# The here-doc exemption applies to the `read` heuristic ONLY, and the
|
||||
# asymmetry is the point. `read` is an ordinary English verb, so any prose a
|
||||
# script prints is one line-wrap away from opening with it. `input(` is not a
|
||||
# word — a line beginning `input(` inside a here-doc is an embedded Python
|
||||
# program pausing for a keypress, which is exactly what this check is for, and
|
||||
# these scripts embed Python in a here-doc as a matter of course. Exempting the
|
||||
# whole body would have disarmed the check across every script in the corpus.
|
||||
def interactive_reads(source):
|
||||
hits = []
|
||||
prev_line = ''
|
||||
for line in source.splitlines():
|
||||
lines = source.splitlines()
|
||||
in_heredoc = heredoc_body_indices(lines)
|
||||
for idx, line in enumerate(lines):
|
||||
stripped = line.strip()
|
||||
if idx in in_heredoc:
|
||||
if re.match(r'input\(', stripped):
|
||||
hits.append(stripped)
|
||||
continue
|
||||
if re.match(r'read(\s|$)', stripped):
|
||||
if not stdin_redirected(line, prev_line):
|
||||
hits.append(stripped)
|
||||
|
||||
Reference in New Issue
Block a user