fix(kyberforge): resolve PR #144 review and audit round 1
- factory-audit: no-op hooks, ./ after interpreters, split-quote and
spaced ${PLUGIN_ROOT} paths, camelCase events in Claude-targeted flat
files, case-insensitive routing stems, and non-string YAML keys are
now caught; input: forms and prompt boundary clauses align with
primitive-author; bats 347 -> 367
- primitive-author: routing forms, quoting guidance, install exit on
hidden Unicode, argument-hint exception
- forge: drop duplicated gotcha, fit description and body budgets (#143)
- skill-author: primitive-author boundary, Claude-only env vars
- hook: exit unless CLAUDE_PROJECT_DIR is set, so Copilot/Codex never
run apm update; ADR-0019 correction, ADR-0025 amendment, docs fixes
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:
@@ -178,32 +178,66 @@ CLAUDE_MAPPED_CAMEL = {'preToolUse', 'postToolUse', 'sessionStart', 'agentStop'}
|
||||
HOOK_COMMAND_KEYS = ('command', 'bash', 'powershell', 'windows', 'linux', 'osx')
|
||||
ROOT_TOKENS = ('PLUGIN_ROOT', 'CLAUDE_PLUGIN_ROOT', 'CURSOR_PLUGIN_ROOT', 'KIRO_PLUGIN_ROOT')
|
||||
ROOT_TOKEN_RE = re.compile(r'\$\{(' + '|'.join(ROOT_TOKENS) + r')\}')
|
||||
# apm 0.28.0's own patterns, hook_integrator.py _rewrite_command_for_target:
|
||||
# the path must follow the token directly and ends at whitespace or a quote.
|
||||
# The ./ pattern is applied with finditer over the whole command, so it
|
||||
# matches after an interpreter (`bash ./x.sh`) too.
|
||||
APM_ROOT_REF_RE = re.compile(r'\$\{(?:' + '|'.join(ROOT_TOKENS) + r')\}([\\/][^\s"\']+)')
|
||||
APM_REL_REF_RE = re.compile(r'(\.[\\/][^\s"\']+)')
|
||||
|
||||
|
||||
def extract_script_refs(cmd):
|
||||
def _is_first(prefix):
|
||||
return not prefix.strip().strip('"\'').strip()
|
||||
|
||||
|
||||
def is_handler(h):
|
||||
# A handler runs something: a command key, or a non-command handler type
|
||||
# (Claude's prompt/agent/http hooks) whose payload is not a script.
|
||||
if not isinstance(h, dict):
|
||||
return False
|
||||
if any(isinstance(h.get(k), str) and h.get(k).strip() for k in HOOK_COMMAND_KEYS):
|
||||
return True
|
||||
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 in a hook command string. kind is 'root' for a ${*_PLUGIN_ROOT}
|
||||
token, 'rel' for a leading ./path."""
|
||||
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."""
|
||||
refs = []
|
||||
masked = cmd
|
||||
for m in ROOT_TOKEN_RE.finditer(cmd):
|
||||
start, end = m.start(), m.end()
|
||||
opened = start > 0 and cmd[start - 1] in '"\''
|
||||
quote = cmd[start - 1] if opened else None
|
||||
rest = cmd[end:]
|
||||
if opened and rest.startswith(quote):
|
||||
# split-quoted form: "${PLUGIN_ROOT}"/scripts/my\ hook.sh
|
||||
rest = rest[1:]
|
||||
path = re.match(r'((?:\\.|[^\s"\'])*)', rest).group(1).replace('\\', '')
|
||||
elif opened:
|
||||
path = rest.split(quote, 1)[0]
|
||||
if end < len(cmd) and cmd[end] in '"\'' and cmd[end + 1:end + 2] in ('/', '\\'):
|
||||
fail(f"script path '{cmd[start:]}' splits the quote after ${{{m.group(1)}}} — apm rewrites only a path that follows the token directly, so this one deploys unrewritten and unbundled; quote the whole token: \"${{PLUGIN_ROOT}}/<path>\" — {where}")
|
||||
for m in APM_ROOT_REF_RE.finditer(cmd):
|
||||
start, end = m.start(), m.end()
|
||||
opener = cmd[start - 1] if start > 0 and cmd[start - 1] in '"\'' else None
|
||||
path = m.group(1)
|
||||
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").
|
||||
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)
|
||||
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:
|
||||
path = re.match(r'((?:\\.|[^\s"\'])*)', rest).group(1).replace('\\', '')
|
||||
prefix = cmd[:start - 1] if opened else cmd[:start]
|
||||
refs.append(('root', path.lstrip('/'), not prefix.strip()))
|
||||
stripped = cmd.lstrip().lstrip('"\'')
|
||||
if stripped.startswith('./'):
|
||||
path = re.match(r'((?:\\.|[^\s"\'])*)', stripped).group(1).replace('\\', '')
|
||||
refs.append(('rel', path, True))
|
||||
prefix = cmd[:start - 1] if opener else cmd[:start]
|
||||
refs.append(('root', path.replace('\\', '/').lstrip('/'), _is_first(prefix)))
|
||||
masked = masked[:start] + ' ' * (end - start) + masked[end:]
|
||||
for m in APM_REL_REF_RE.finditer(masked):
|
||||
start = m.start()
|
||||
ref = m.group(1)
|
||||
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
|
||||
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)))
|
||||
return refs
|
||||
|
||||
|
||||
@@ -267,6 +301,45 @@ def check_script(kind_, rel, first, pkg_root, where):
|
||||
fail(f"script '{rel}' is run directly but is not executable — chmod +x it, or invoke it through an interpreter — {where}")
|
||||
|
||||
|
||||
def owning_apm_yml():
|
||||
"""The nearest apm.yml walking up from the hook file, or None."""
|
||||
d = parent_dir
|
||||
while True:
|
||||
cand = os.path.join(d, 'apm.yml')
|
||||
if os.path.isfile(cand):
|
||||
return cand
|
||||
up = os.path.dirname(d)
|
||||
if up == d:
|
||||
return None
|
||||
d = up
|
||||
|
||||
|
||||
def targets_claude():
|
||||
"""Whether apm renders this package's hooks to Claude. 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."""
|
||||
path = owning_apm_yml()
|
||||
if path is None:
|
||||
return True
|
||||
try:
|
||||
with open(path, encoding='utf-8') as f:
|
||||
data = yaml.safe_load(f)
|
||||
except (OSError, UnicodeDecodeError, yaml.YAMLError):
|
||||
return True
|
||||
if not isinstance(data, dict):
|
||||
return True
|
||||
raw = data.get('targets', data.get('target'))
|
||||
if raw is None:
|
||||
return True
|
||||
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
|
||||
|
||||
|
||||
def audit_hook():
|
||||
check_not_linked(hardlinks=False)
|
||||
stem = fname[:-len('.json')]
|
||||
@@ -280,7 +353,8 @@ def audit_hook():
|
||||
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}")
|
||||
|
||||
if ROUTING_STEM_RE.search(stem):
|
||||
# apm lowercases the stem before routing (hook_file_routing.py).
|
||||
if ROUTING_STEM_RE.search(stem.lower()):
|
||||
suggest(f"filename stem '{stem}' uses deprecated hook filename routing — name it plainly and narrow reach with target:/targets: in the package's apm.yml — {fname}")
|
||||
|
||||
content = read_text(target)
|
||||
@@ -334,13 +408,31 @@ def audit_hook():
|
||||
elif 'command' in entry:
|
||||
claude_shaped = True
|
||||
|
||||
if shape_ok:
|
||||
# hook.md Must 3: the file contributes at least one entry. An empty
|
||||
# event list, or an entry with no handler, deploys nothing runnable.
|
||||
total = 0
|
||||
for event, entries in events.items():
|
||||
for i, entry in enumerate(entries):
|
||||
total += 1
|
||||
handlers = entry['hooks'] if 'hooks' in entry else [entry]
|
||||
if not any(is_handler(h) for h in handlers):
|
||||
fail(f"event '{event}' entry {i} has no handler — no command (or other handler type) to run, so it deploys nothing — {fname}")
|
||||
if total == 0:
|
||||
fail(f"contributes no hook entries — every event list is empty, so apm deploys nothing — {fname}")
|
||||
|
||||
# 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()
|
||||
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}")
|
||||
elif claude_shaped and event[0].islower() and event not in CLAUDE_MAPPED_CAMEL:
|
||||
fail(f"event '{event}' is camelCase in a Claude-shaped file and Claude's map does not rename it — it deploys verbatim and never fires; 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}")
|
||||
|
||||
if not shape_ok:
|
||||
return
|
||||
@@ -357,7 +449,7 @@ def audit_hook():
|
||||
continue
|
||||
if '${CLAUDE_PLUGIN_ROOT}' in cmd:
|
||||
uses_claude_token = True
|
||||
refs = extract_script_refs(cmd)
|
||||
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):
|
||||
@@ -446,7 +538,7 @@ def audit_instruction():
|
||||
elif apply_to_ok and isinstance(apply_to, list):
|
||||
suggest(f"applyTo is a YAML list — Copilot receives the file verbatim and its handling of a list is unverified; use one comma-separated string — {fname}")
|
||||
|
||||
extra = sorted(k for k in fm if k not in INSTRUCTION_KEYS)
|
||||
extra = sorted(str(k) for k in fm if k not in INSTRUCTION_KEYS)
|
||||
if extra:
|
||||
suggest(f"frontmatter key(s) {', '.join(extra)} are read by no target and dropped on Claude — keep to description and applyTo (author, version optional) — {fname}")
|
||||
|
||||
@@ -461,6 +553,8 @@ INPUT_NAME_RE = re.compile(r'^[A-Za-z][\w-]{0,63}$')
|
||||
# apm's own rewrite pattern for ${input:x}, command_integrator.py.
|
||||
INPUT_REF_RE = re.compile(r'\$\{\{?\s*input\s*:\s*([\w-]+)\s*\}?\}')
|
||||
TRIGGER_RE = re.compile(r'\buse\s+(?:this\s+)?when\b', re.IGNORECASE)
|
||||
# The skill boundary form `Not <thing> -> <target>` (ASCII or Unicode arrow).
|
||||
BOUNDARY_RE = re.compile(r'\bnot\b[^.;]*?(?:->|\u2192)', re.IGNORECASE)
|
||||
PROMPT_DESC_SUGGEST_CHARS = 250
|
||||
|
||||
|
||||
@@ -481,15 +575,20 @@ def prompt_input_names(spec):
|
||||
return
|
||||
names.append(s)
|
||||
|
||||
form = "write each input as `- <name>: \"<description>\"` (primitive-author prompt Must 3)"
|
||||
if spec is None:
|
||||
return names
|
||||
if isinstance(spec, str):
|
||||
fail(f"input: is a bare name, not the object form — {form}, so every argument carries its description — {fname}")
|
||||
accept(spec)
|
||||
elif isinstance(spec, dict):
|
||||
fail(f"input: is a map, not the object form — {form} — {fname}")
|
||||
for k in spec:
|
||||
accept(k)
|
||||
elif isinstance(spec, list):
|
||||
for item in spec:
|
||||
if not isinstance(item, dict):
|
||||
fail(f"input entry {item!r} is a bare name, not the object form — {form} — {fname}")
|
||||
if isinstance(item, dict):
|
||||
if len(item) > 1:
|
||||
keys = ', '.join(str(k) for k in item)
|
||||
@@ -532,11 +631,13 @@ def audit_prompt():
|
||||
suggest(f"description is {len(desc)} characters (> {PROMPT_DESC_SUGGEST_CHARS}) — it is one user-facing sentence (ADR-0029) — {fname}")
|
||||
if TRIGGER_RE.search(desc):
|
||||
suggest(f"description carries a 'Use when' trigger clause — a prompt is user-triggered (ADR-0029); a trigger clause invites the model to route to it on Claude — {fname}")
|
||||
if BOUNDARY_RE.search(desc):
|
||||
suggest(f"description carries a 'Not X -> Y' boundary clause — a prompt is user-triggered (ADR-0029, primitive-author prompt Should 6); name the skills it steers instead — {fname}")
|
||||
|
||||
for camel, kebab in PROMPT_CAMEL_ALIASES.items():
|
||||
if camel in fm:
|
||||
suggest(f"'{camel}' — use the kebab-case spelling '{kebab}' apm documents — {fname}")
|
||||
extra = sorted(k for k in fm if k not in PROMPT_KEYS and k not in PROMPT_CAMEL_ALIASES)
|
||||
extra = sorted(str(k) for k in fm if k not in PROMPT_KEYS and k not in PROMPT_CAMEL_ALIASES)
|
||||
if extra:
|
||||
suggest(f"frontmatter key(s) {', '.join(extra)} are dropped on Claude (it keeps only {', '.join(sorted(PROMPT_KEYS))}) — keep them only if the Copilot-only behaviour is intended — {fname}")
|
||||
|
||||
@@ -559,15 +660,19 @@ def audit_prompt():
|
||||
suggest(f"argument-hint is set alongside input: — apm synthesises the hint from input: names; drop it unless that form is inadequate — {fname}")
|
||||
|
||||
|
||||
if kind == 'hook':
|
||||
audit_hook()
|
||||
elif kind == 'instruction':
|
||||
audit_instruction()
|
||||
elif kind == 'prompt':
|
||||
audit_prompt()
|
||||
else:
|
||||
AUDITS = {'hook': lambda: audit_hook(), 'instruction': lambda: audit_instruction(), 'prompt': lambda: audit_prompt()}
|
||||
if kind not in AUDITS:
|
||||
print(f"Error: unknown primitive kind '{kind}'", file=sys.stderr)
|
||||
sys.exit(2)
|
||||
try:
|
||||
AUDITS[kind]()
|
||||
except Exception as exc: # an input shape no check anticipated
|
||||
# Exit 1 means findings; a crash means the checks never completed, which
|
||||
# is the never-ran tier, not a verdict on the file.
|
||||
print(f"Error: the {kind} checks crashed ({type(exc).__name__}: {exc}) and did not complete — {fname}", file=sys.stderr)
|
||||
print(" Why: a partial run reported as findings (exit 1) or as clean (exit 0) would be a verdict the checks never reached.", file=sys.stderr)
|
||||
print(" Fix: report ### Structure as unverified, and file the input shape against factory-audit's lib-checks-primitive.sh.", file=sys.stderr)
|
||||
sys.exit(2)
|
||||
|
||||
for s in suggestions:
|
||||
print(f"SUGGESTION {s}")
|
||||
|
||||
@@ -196,7 +196,7 @@ TARGET="$1"
|
||||
if [[ ! -e "$TARGET" && ! -L "$TARGET" ]]; then
|
||||
echo "Error: '$TARGET' does not exist." >&2
|
||||
echo " Why: the path shape says what would be audited, but there is nothing at this path to audit — and auditing a target that is not there would report the absence as findings about it, sending the reader after a spec violation instead of a typo." >&2
|
||||
echo " Fix: check the path, and pass an existing skill directory (or its SKILL.md) or an existing agent file." >&2
|
||||
echo " Fix: check the path, and pass an existing skill directory (or its SKILL.md), agent file, hook file (.json under a hooks/ directory), *.instructions.md or *.prompt.md." >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
@@ -212,8 +212,8 @@ if [[ -d "$TARGET" ]]; then
|
||||
MODE=skill
|
||||
else
|
||||
echo "Error: '$TARGET' is a directory with no SKILL.md in it." >&2
|
||||
echo " Why: a skill directory is identified by its SKILL.md, and an agent target is a file, never a directory — so this path matches neither mode and guessing one would report findings of the wrong kind." >&2
|
||||
echo " Fix: pass the skill directory that holds SKILL.md, or an agent file (<name>.agent.md, or a .md file under an agents/ directory)." >&2
|
||||
echo " Why: a skill directory is identified by its SKILL.md, and every other target (agent, hook, instruction, prompt) is a file, never a directory — so this path matches no mode and guessing one would report findings of the wrong kind." >&2
|
||||
echo " Fix: pass the skill directory that holds SKILL.md, an agent file (<name>.agent.md, or a .md file under an agents/ directory), a hook file (.json under a hooks/ directory), a *.instructions.md or a *.prompt.md." >&2
|
||||
exit 2
|
||||
fi
|
||||
elif [[ "$TARGET_BASE" == "SKILL.md" ]]; then
|
||||
|
||||
Reference in New Issue
Block a user