fix(kyberforge): resolve PR #144 review and audit round 2
- factory-audit: ./ and bare/absolute script checks scoped to command position (no false FAILs on ./src or printf); hook sources limited to .apm/hooks or package-root hooks/; Kiro-aware lowercase events; unfilled template placeholders FAIL; repo-only instructions FAIL at any scope; Vale description FAIL documented; bats 367 -> 378 - primitive-author: split-quote/spaced paths and handler-less entries promoted to Must; Step 4.2 renders into a scratch consumer instead of a no-op dry run; dispatch and gate hand-off trimmed - apm-workflow 1.0.2: mutual boundary with primitive-author - forge: no double package bump; gotcha wording - skill-author: create keeps seeded 0.1.0 (ADR-0022); portable, retry-safe new-skill.sh; template and flow consistency fixes - hook docs: cite the ADR-0019 correction; guard caveat Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkT7RSDwDbmrM9T34b6sTi
This commit is contained in:
@@ -13,16 +13,16 @@
|
||||
# description or body, and never validates a prompt's input: names against its
|
||||
# ${input:x} references — so `apm install` and `apm compile --validate` both exit
|
||||
# 0 on files that deploy nothing, or deploy something that never fires. The
|
||||
# checks follow the Authoring checklists at the end of
|
||||
# plugins/kyberforge/docs/research/docs/microsoft-apm/{hooks,instructions,prompt}-primitive-schema.md,
|
||||
# which trace each rule to the apm source that makes it matter, except where
|
||||
# references/{hook,instruction,prompt}-flow.md documents a deliberate deviation
|
||||
# (a tier moved, or a check the research leaves audit-only). A Must in
|
||||
# primitive-author is a FAIL here, a Should a SUGGESTION.
|
||||
# checks follow primitive-author's hook, instruction and prompt reference
|
||||
# checklists (research provenance: source key apm-cli-installed-source in
|
||||
# references/sources.md), except where references/{hook,instruction,prompt}-flow.md
|
||||
# documents a deliberate deviation (a tier moved, or a check the author leaves
|
||||
# audit-only). A Must in primitive-author is a FAIL here, a Should a SUGGESTION.
|
||||
#
|
||||
# No boundary resolver and no word budgets: none of these files is routed on a
|
||||
# description the way a skill is. A prompt's description IS model-visible on
|
||||
# Claude, which is why it gets the two ADR-0029 SUGGESTIONs below — but whether
|
||||
# Claude, which is why it gets the three ADR-0029 description SUGGESTIONs below
|
||||
# (length, trigger clause, boundary clause) — but whether
|
||||
# a prompt body carries procedure that belongs in a skill is a judgment call the
|
||||
# prompt flow makes by reading it, and deliberately has no heuristic here.
|
||||
#
|
||||
@@ -59,6 +59,7 @@ import sys
|
||||
import os
|
||||
import re
|
||||
import json
|
||||
import shlex
|
||||
|
||||
import yaml
|
||||
|
||||
@@ -186,8 +187,32 @@ APM_ROOT_REF_RE = re.compile(r'\$\{(?:' + '|'.join(ROOT_TOKENS) + r')\}([\\/][^\
|
||||
APM_REL_REF_RE = re.compile(r'(\.[\\/][^\s"\']+)')
|
||||
|
||||
|
||||
def _is_first(prefix):
|
||||
return not prefix.strip().strip('"\'').strip()
|
||||
# An interpreter whose first argument is the script it runs. A reference in
|
||||
# that argument slot is in command position just as a first token is.
|
||||
INTERPRETERS = {'bash', 'sh', 'zsh', 'python', 'python3', 'node', 'pwsh', 'ruby', 'perl'}
|
||||
|
||||
|
||||
def _prefix_tokens(prefix):
|
||||
return [t.strip('"\'') for t in prefix.split()]
|
||||
|
||||
|
||||
def _interp_arg_index(tokens):
|
||||
"""Index of the token that is the first argument after a known interpreter
|
||||
(optionally behind `env`), or None when the command does not open with one."""
|
||||
i = 0
|
||||
if tokens and os.path.basename(tokens[0]) == 'env':
|
||||
i = 1
|
||||
if len(tokens) > i and os.path.basename(tokens[i]) in INTERPRETERS:
|
||||
return i + 1
|
||||
return None
|
||||
|
||||
|
||||
def _position(prefix):
|
||||
"""(is_first_token, is_interpreter_arg) for a reference preceded by prefix."""
|
||||
toks = [t for t in _prefix_tokens(prefix) if t]
|
||||
if not toks:
|
||||
return True, False
|
||||
return False, _interp_arg_index(toks) == len(toks)
|
||||
|
||||
|
||||
def is_handler(h):
|
||||
@@ -200,12 +225,13 @@ def is_handler(h):
|
||||
return h.get('type') not in (None, 'command')
|
||||
|
||||
|
||||
def extract_script_refs(cmd, where):
|
||||
"""Yield (kind, relpath, is_first_token) for each package-relative script
|
||||
reference apm would rewrite, reading the command exactly as apm does. kind
|
||||
is 'root' for a ${*_PLUGIN_ROOT} token, 'rel' for a ./path. A token apm
|
||||
reads wrongly — split-quoted, or a path with a space — is a FAIL here,
|
||||
because apm leaves it unrewritten or cuts it short."""
|
||||
def extract_script_refs(cmd, pkg_root, where):
|
||||
"""Return (kind, relpath, is_first_token, is_interpreter_arg) for each
|
||||
package-relative reference apm would rewrite, reading the command exactly
|
||||
as apm does. kind is 'root' for a ${*_PLUGIN_ROOT} token, 'rel' for a
|
||||
./path, 'up' for a ../path. A token apm reads wrongly — split-quoted, or a
|
||||
path with a space — is a FAIL here, because apm leaves it unrewritten or
|
||||
cuts it short."""
|
||||
refs = []
|
||||
masked = cmd
|
||||
for m in ROOT_TOKEN_RE.finditer(cmd):
|
||||
@@ -219,34 +245,42 @@ def extract_script_refs(cmd, where):
|
||||
nxt = cmd[end:end + 1]
|
||||
# A backslash-escaped space, or a quoted token whose script name only
|
||||
# completes past the whitespace apm stopped at ("…/my hook.sh").
|
||||
# A path apm read that exists as a file is exactly what apm bundles, so
|
||||
# a later argument inside the same quotes (`bash -c "…/tool --x a.sh"`)
|
||||
# is an argument, not the rest of a spaced name.
|
||||
spaced = nxt.isspace() and path.endswith('\\')
|
||||
if not spaced and opener is not None and nxt.isspace():
|
||||
quoted = cmd[start:].split(opener, 1)[0]
|
||||
spaced = bool(SCRIPT_EXT_RE.search(quoted)) and not SCRIPT_EXT_RE.search(path)
|
||||
exists = os.path.isfile(os.path.join(pkg_root, path.replace('\\', '/').lstrip('/')))
|
||||
spaced = (not exists and bool(SCRIPT_EXT_RE.search(quoted))
|
||||
and not SCRIPT_EXT_RE.search(path))
|
||||
if spaced:
|
||||
fail(f"script path '{cmd[start:]}' contains a space — apm reads a ${{PLUGIN_ROOT}} path only up to the first whitespace or quote, so it bundles the wrong file and the hook fails; rename the script without spaces — {where}")
|
||||
else:
|
||||
prefix = cmd[:start - 1] if opener else cmd[:start]
|
||||
refs.append(('root', path.replace('\\', '/').lstrip('/'), _is_first(prefix)))
|
||||
refs.append(('root', path.replace('\\', '/').lstrip('/')) + _position(prefix))
|
||||
masked = masked[:start] + ' ' * (end - start) + masked[end:]
|
||||
for m in APM_REL_REF_RE.finditer(masked):
|
||||
start = m.start()
|
||||
ref = m.group(1)
|
||||
kind_ = 'rel'
|
||||
if start > 0 and masked[start - 1] == '.':
|
||||
fail(f"script path '..{ref[1:]}' starts with ../ — apm reads it as ./{ref[2:]} from the hook directory, not the parent, so the wrong file (or none) is bundled; reference it as ${{PLUGIN_ROOT}}/<path> — {where}")
|
||||
continue
|
||||
kind_, start = 'up', start - 1
|
||||
opener = masked[start - 1] if start > 0 and masked[start - 1] in '"\'' else None
|
||||
prefix = masked[:start - 1] if opener else masked[:start]
|
||||
refs.append(('rel', ref[2:].replace('\\', '/'), _is_first(prefix)))
|
||||
refs.append((kind_, ref[2:].replace('\\', '/')) + _position(prefix))
|
||||
return refs
|
||||
|
||||
|
||||
SCRIPT_EXT_RE = re.compile(r'\.(?:sh|bash|zsh|py|js|mjs|cjs|ts|ps1|rb|pl)$', re.IGNORECASE)
|
||||
|
||||
|
||||
def first_token(cmd):
|
||||
m = re.match(r'\s*(["\']?)((?:\\.|[^\s"\'])*)\1', cmd)
|
||||
return m.group(2).replace('\\', '') if m else ''
|
||||
def command_tokens(cmd):
|
||||
"""The command's leading whitespace-delimited tokens, quotes removed."""
|
||||
try:
|
||||
return shlex.split(cmd)
|
||||
except ValueError:
|
||||
return _prefix_tokens(cmd)
|
||||
|
||||
|
||||
def check_unanchored_script(cmd, pkg_root, where):
|
||||
@@ -255,26 +289,53 @@ def check_unanchored_script(cmd, pkg_root, where):
|
||||
# fine. An absolute script path, or a bare relative path to a file in the
|
||||
# package, also passes through untouched — so the script is not bundled
|
||||
# and the deployed hook points at a path that does not exist on the
|
||||
# consumer's machine.
|
||||
tok = first_token(cmd)
|
||||
if not tok or tok.startswith('./') or '$' in tok or tok.startswith('~'):
|
||||
# consumer's machine. Checked in command position only: the first token,
|
||||
# and the first argument after a known interpreter (`bash scripts/x.sh`).
|
||||
# A later argument is data, not a script apm is asked to run.
|
||||
toks = command_tokens(cmd)
|
||||
if not toks:
|
||||
return
|
||||
if tok.startswith('/'):
|
||||
real_root = os.path.realpath(pkg_root)
|
||||
inside = os.path.realpath(tok).startswith(real_root + os.sep)
|
||||
if inside or SCRIPT_EXT_RE.search(tok):
|
||||
fail(f"script '{tok}' is an absolute path — apm neither bundles nor rewrites it, so it breaks on every other machine; reference it as ${{PLUGIN_ROOT}}/<path> — {where}")
|
||||
return
|
||||
if '/' in tok:
|
||||
for base in (parent_dir, pkg_root):
|
||||
if os.path.isfile(os.path.join(base, tok)):
|
||||
fail(f"script '{tok}' is a bare relative path — apm bundles and rewrites only ${{PLUGIN_ROOT}}/... and ./... references, so this one deploys unbundled; prefix it with ${{PLUGIN_ROOT}}/ or ./ — {where}")
|
||||
return
|
||||
slots = [0]
|
||||
arg = _interp_arg_index(toks)
|
||||
if arg is not None and arg < len(toks):
|
||||
slots.append(arg)
|
||||
for idx in slots:
|
||||
tok = toks[idx]
|
||||
if not tok or tok.startswith(('./', '../', '~', '-')) or '$' in tok:
|
||||
continue
|
||||
if tok.startswith('/'):
|
||||
real_root = os.path.realpath(pkg_root)
|
||||
inside = os.path.realpath(tok).startswith(real_root + os.sep)
|
||||
# In the first slot an extension-less absolute path outside the
|
||||
# package (`/usr/bin/env`, `/bin/bash`) is the host's interpreter;
|
||||
# in the interpreter-argument slot it is the script being run.
|
||||
if inside or SCRIPT_EXT_RE.search(tok) or idx > 0:
|
||||
fail(f"script '{tok}' is an absolute path — apm neither bundles nor rewrites it, so it breaks on every other machine; reference it as ${{PLUGIN_ROOT}}/<path> — {where}")
|
||||
continue
|
||||
if '/' in tok:
|
||||
for base in (parent_dir, pkg_root):
|
||||
if os.path.isfile(os.path.join(base, tok)):
|
||||
fail(f"script '{tok}' is a bare relative path — apm bundles and rewrites only ${{PLUGIN_ROOT}}/... and ./... references, so this one deploys unbundled; prefix it with ${{PLUGIN_ROOT}}/ or ./ — {where}")
|
||||
break
|
||||
|
||||
|
||||
def check_script(kind_, rel, first, pkg_root, where):
|
||||
def check_script(kind_, rel, first, interp_arg, pkg_root, where):
|
||||
if not rel:
|
||||
return
|
||||
# apm's ./ pattern also matches plain arguments — a cwd directory
|
||||
# (`npx prettier --check ./src`), a printf escape (`'.\\n'`), a sibling path.
|
||||
# apm only warns on those and they run against the consumer's cwd as
|
||||
# meant, so a ./ or ../ match is held to the script rules only in command
|
||||
# position or when it names a script by extension (or a package entry that
|
||||
# is not a file).
|
||||
strong = kind_ == 'root' or first or interp_arg or bool(SCRIPT_EXT_RE.search(rel))
|
||||
if kind_ == 'up':
|
||||
in_pkg = os.path.exists(os.path.join(pkg_root, rel)) and not os.path.isfile(os.path.join(pkg_root, rel))
|
||||
if strong or in_pkg:
|
||||
fail(f"script path '../{rel}' starts with ../ — apm reads it as ./{rel} from the hook directory, not the parent, so the wrong file (or none) is bundled; reference it as ${{PLUGIN_ROOT}}/<path> — {where}")
|
||||
else:
|
||||
suggest(f"argument '../{rel}' matches apm's ./ script pattern — apm will warn 'Hook script not found' and leave it unrewritten; harmless if it is a path in the consumer's working directory — {where}")
|
||||
return
|
||||
if '$' in rel or '`' in rel:
|
||||
fail(f"script path '{rel}' contains '$' or a backtick — apm refuses to rewrite it for Claude — {where}")
|
||||
return
|
||||
@@ -295,7 +356,12 @@ def check_script(kind_, rel, first, pkg_root, where):
|
||||
found = c
|
||||
break
|
||||
if found is None:
|
||||
fail(f"script '{rel}' does not exist in the package — apm only warns, then deploys a hook that fails every time it fires — {where}")
|
||||
not_a_file = any(os.path.exists(c) for c in candidates)
|
||||
if strong or not_a_file:
|
||||
what = "exists in the package but is not a regular file" if not_a_file else "does not exist in the package"
|
||||
fail(f"script '{rel}' {what} — apm only warns, then deploys a hook that fails every time it fires — {where}")
|
||||
else:
|
||||
suggest(f"argument './{rel}' matches apm's ./ script pattern but names no package file — apm will warn 'Hook script not found' and leave it unrewritten; harmless if it is a path in the consumer's working directory — {where}")
|
||||
return
|
||||
if first and not os.access(found, os.X_OK):
|
||||
fail(f"script '{rel}' is run directly but is not executable — chmod +x it, or invoke it through an interpreter — {where}")
|
||||
@@ -314,44 +380,73 @@ def owning_apm_yml():
|
||||
d = up
|
||||
|
||||
|
||||
def targets_claude():
|
||||
"""Whether apm renders this package's hooks to Claude. Mirrors
|
||||
def package_targets():
|
||||
"""The set of targets apm renders this package's hooks to. Mirrors
|
||||
parse_targets_field: no target:/targets: (or no apm.yml) means every
|
||||
target, and 'all' folds to every target. An unreadable apm.yml is treated
|
||||
as every target, the reading that keeps the stricter check on."""
|
||||
as every target, the reading that keeps the stricter checks on."""
|
||||
every = set(ROUTING_TOKENS)
|
||||
path = owning_apm_yml()
|
||||
if path is None:
|
||||
return True
|
||||
return every
|
||||
try:
|
||||
with open(path, encoding='utf-8') as f:
|
||||
data = yaml.safe_load(f)
|
||||
except (OSError, UnicodeDecodeError, yaml.YAMLError):
|
||||
return True
|
||||
return every
|
||||
if not isinstance(data, dict):
|
||||
return True
|
||||
return every
|
||||
raw = data.get('targets', data.get('target'))
|
||||
if raw is None:
|
||||
return True
|
||||
return every
|
||||
if isinstance(raw, list):
|
||||
tokens = [str(t).strip().lower() for t in raw]
|
||||
else:
|
||||
tokens = [t.strip().lower() for t in str(raw).split(',')]
|
||||
tokens = [t for t in tokens if t]
|
||||
return not tokens or 'claude' in tokens or 'all' in tokens
|
||||
tokens = {t for t in tokens if t}
|
||||
if not tokens or 'all' in tokens:
|
||||
return every
|
||||
return tokens
|
||||
|
||||
|
||||
# apm 0.28.0 _HOOK_EVENT_MAP: the only all-lowercase source name any target
|
||||
# renames is Kiro's `stop` -> `Stop`. Every other target deploys an
|
||||
# all-lowercase name verbatim, where it never fires.
|
||||
LOWERCASE_EVENT_TARGETS = {'stop': {'kiro'}}
|
||||
|
||||
# The directories apm deploys hooks into for each harness. A hook file under
|
||||
# one of them is install output, not package source.
|
||||
DEPLOY_ROOTS = ('.github', '.claude', '.cursor', '.codex', '.kiro', '.windsurf',
|
||||
'.gemini', '.vscode', '.antigravity', '.copilot')
|
||||
PLACEHOLDER_RE = re.compile(r'FILL IN|FILL_IN_')
|
||||
|
||||
|
||||
def check_placeholders(content):
|
||||
m = PLACEHOLDER_RE.search(content)
|
||||
if m:
|
||||
line = content.count('\n', 0, m.start()) + 1
|
||||
fail(f"unfilled template placeholder '{m.group(0)}' at line {line} — primitive-author Step 3 fills every FILL IN and FILL_IN_ placeholder before the file ships — {fname}")
|
||||
|
||||
|
||||
def audit_hook():
|
||||
check_not_linked(hardlinks=False)
|
||||
stem = fname[:-len('.json')]
|
||||
hooks_dir = os.path.basename(parent_dir)
|
||||
if hooks_dir != 'hooks':
|
||||
fail(f"is not directly in a hooks/ directory — apm discovers hook files only at .apm/hooks/*.json and hooks/*.json, non-recursively — {fname}")
|
||||
if os.path.basename(os.path.dirname(parent_dir)) == '.apm':
|
||||
pkg_root = os.path.dirname(os.path.dirname(parent_dir))
|
||||
# validate.sh dispatches only a .json directly under a hooks/ directory.
|
||||
# apm discovers package source at .apm/hooks/*.json and at a package-root
|
||||
# hooks/*.json; anything else under a hooks/ directory is apm's deployed
|
||||
# output (.github/hooks/, .cursor/hooks/, ...) or not a package at all.
|
||||
above = os.path.dirname(parent_dir)
|
||||
if os.path.basename(above) == '.apm':
|
||||
pkg_root = os.path.dirname(above)
|
||||
if not os.path.isfile(os.path.join(pkg_root, 'apm.yml')):
|
||||
info(f"no apm.yml at the inferred package root {pkg_root} — script paths are resolved against it anyway — {fname}")
|
||||
elif os.path.isfile(os.path.join(above, 'apm.yml')):
|
||||
pkg_root = above
|
||||
else:
|
||||
pkg_root = os.path.dirname(parent_dir)
|
||||
if not os.path.isfile(os.path.join(pkg_root, 'apm.yml')):
|
||||
info(f"no apm.yml at the inferred package root {pkg_root} — script paths are resolved against it anyway — {fname}")
|
||||
kind_of = (f"apm's deployed output ({os.path.basename(above)}/hooks/)"
|
||||
if os.path.basename(above) in DEPLOY_ROOTS else 'no package source')
|
||||
fail(f"is {kind_of} — apm reads hook source only from <package>/.apm/hooks/*.json or a package-root hooks/*.json beside apm.yml; audit the source file in the package's .apm/hooks/ instead — {fname}")
|
||||
return
|
||||
|
||||
# apm lowercases the stem before routing (hook_file_routing.py).
|
||||
if ROUTING_STEM_RE.search(stem.lower()):
|
||||
@@ -360,6 +455,7 @@ def audit_hook():
|
||||
content = read_text(target)
|
||||
if content is None:
|
||||
return
|
||||
check_placeholders(content)
|
||||
try:
|
||||
doc = json.loads(content)
|
||||
except json.JSONDecodeError as exc:
|
||||
@@ -424,12 +520,18 @@ def audit_hook():
|
||||
# camelCase outside Claude's map never fires on Claude. A flat
|
||||
# Copilot-shaped file still renders to Claude whenever the package targets
|
||||
# it, so the exemption holds only for a package that does not.
|
||||
camel_checked = claude_shaped or targets_claude()
|
||||
deploys_to = package_targets()
|
||||
camel_checked = claude_shaped or 'claude' in deploys_to
|
||||
for event in events:
|
||||
if not event.strip():
|
||||
fail(f"empty event name — {fname}")
|
||||
elif not any(c.isupper() for c in event):
|
||||
fail(f"event '{event}' is all-lowercase — no target maps it and apm never warns, so it silently never fires — {fname}")
|
||||
mapping = LOWERCASE_EVENT_TARGETS.get(event, set())
|
||||
unmapped = sorted(deploys_to - mapping)
|
||||
if not mapping & deploys_to:
|
||||
fail(f"event '{event}' is all-lowercase — no target this package deploys to maps it, and apm never warns, so it silently never fires; write it in PascalCase — {fname}")
|
||||
elif unmapped:
|
||||
suggest(f"event '{event}' is all-lowercase — only Kiro renames it; {', '.join(unmapped)} receive it verbatim and it never fires there; write it in PascalCase — {fname}")
|
||||
elif camel_checked and event[0].islower() and event not in CLAUDE_MAPPED_CAMEL:
|
||||
why = 'in a Claude-shaped file' if claude_shaped else "and the package's apm.yml targets Claude (no targets: means every target)"
|
||||
fail(f"event '{event}' is camelCase {why}, and Claude's map does not rename it — it deploys verbatim to Claude and never fires; write it in PascalCase — {fname}")
|
||||
@@ -449,11 +551,9 @@ def audit_hook():
|
||||
continue
|
||||
if '${CLAUDE_PLUGIN_ROOT}' in cmd:
|
||||
uses_claude_token = True
|
||||
refs = extract_script_refs(cmd, where)
|
||||
for kind_, rel, first in refs:
|
||||
check_script(kind_, rel, first, pkg_root, where)
|
||||
if not any(first for _, _, first in refs):
|
||||
check_unanchored_script(cmd, pkg_root, where)
|
||||
for kind_, rel, first, interp_arg in extract_script_refs(cmd, pkg_root, where):
|
||||
check_script(kind_, rel, first, interp_arg, pkg_root, where)
|
||||
check_unanchored_script(cmd, pkg_root, where)
|
||||
|
||||
if uses_claude_token:
|
||||
suggest(f"uses ${{CLAUDE_PLUGIN_ROOT}} — apm documents the target-neutral ${{PLUGIN_ROOT}}, which it rewrites identically for every target — {fname}")
|
||||
@@ -524,6 +624,7 @@ def audit_instruction():
|
||||
content = read_text(target)
|
||||
if content is None:
|
||||
return
|
||||
check_placeholders(content)
|
||||
fm, body, ok = split_frontmatter(content)
|
||||
if not ok:
|
||||
return
|
||||
@@ -621,6 +722,7 @@ def audit_prompt():
|
||||
content = read_text(target)
|
||||
if content is None:
|
||||
return
|
||||
check_placeholders(content)
|
||||
fm, body, ok = split_frontmatter(content)
|
||||
if not ok:
|
||||
return
|
||||
|
||||
@@ -131,7 +131,8 @@ the target:
|
||||
directory is named 'agents' (.apm/agents, .claude/agents,
|
||||
.github/agents, .copilot/agents).
|
||||
hook mode the target is a *.json file directly under a hooks/
|
||||
directory (.apm/hooks, or a package's root hooks/).
|
||||
directory (.apm/hooks, or a hooks/ beside apm.yml; any
|
||||
other hooks/ directory is apm's deployed output and FAILs).
|
||||
instruction mode the target is a *.instructions.md file.
|
||||
prompt mode the target is a *.prompt.md file.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user